Skip to content

Fix mismatched doc comments on exported weight config constants#204

Open
ilopezluna wants to merge 1 commit intomodelpack:mainfrom
ilopezluna:fix/mediatype-comment-names
Open

Fix mismatched doc comments on exported weight config constants#204
ilopezluna wants to merge 1 commit intomodelpack:mainfrom
ilopezluna:fix/mediatype-comment-names

Conversation

@ilopezluna
Copy link
Copy Markdown

Description

Fix Go doc comments for three exported constants in specs-go/v1/mediatype.go where the comment name did not match the actual constant name:

  • MediaTypeModelWeightConfig — comment incorrectly started with MediaTypeModelConfig
  • MediaTypeModelWeightConfigGzip — comment incorrectly started with MediaTypeModelConfigGzip
  • MediaTypeModelWeightConfigZstd — comment incorrectly started with MediaTypeModelConfigZstd

All three comments were missing the Weight segment in the identifier name, causing them to violate Go's convention that doc comments on exported identifiers should start with the identifier's name.

Related Issue

No existing issue. This was identified by reviewing the doc comments in the mediatype.go file.

Motivation and Context

Go doc comments on exported identifiers should start with the identifier name (per Go conventions and go vet/revive best practices). These mismatched comments could cause confusion for contributors and consumers of the package, especially when relying on generated documentation (e.g., pkg.go.dev), where the comment text would reference a non-existent identifier.

Signed-off-by: Ignacio López Luna <ignasi.lopez.luna@gmail.com>
@ilopezluna ilopezluna force-pushed the fix/mediatype-comment-names branch from ff2cc24 to be3e085 Compare April 9, 2026 08:12
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the documentation comments for several media type constants to correctly match their identifiers. The review feedback suggests further refining the phrasing of these comments to improve consistency with other definitions in the codebase.

MediaTypeModelWeightConfigRaw = "application/vnd.cncf.model.weight.config.v1.raw"

// MediaTypeModelConfig specifies the media type for configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
// MediaTypeModelWeightConfig specifies the media type for configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For better consistency with the MediaTypeModelConfig definition (line 25) and the constant name itself, consider using the phrasing 'model weight configuration' instead of 'configuration of the model weights'. This also aligns with the singular 'weight' used in the identifier and media type string.

Suggested change
// MediaTypeModelWeightConfig specifies the media type for configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
// MediaTypeModelWeightConfig specifies the media type for a model weight configuration, including files like tokenizer.json, config.json, etc.

MediaTypeModelWeightConfig = "application/vnd.cncf.model.weight.config.v1.tar"

// MediaTypeModelConfigGzip specifies the media type for gzipped configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
// MediaTypeModelWeightConfigGzip specifies the media type for gzipped configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with the base constant and other compressed media types in this file, consider using 'gzipped model weight configuration'.

Suggested change
// MediaTypeModelWeightConfigGzip specifies the media type for gzipped configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
// MediaTypeModelWeightConfigGzip specifies the media type for a gzipped model weight configuration, including files like tokenizer.json, config.json, etc.

MediaTypeModelWeightConfigGzip = "application/vnd.cncf.model.weight.config.v1.tar+gzip"

// MediaTypeModelConfigZstd specifies the media type for zstd compressed configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
// MediaTypeModelWeightConfigZstd specifies the media type for zstd compressed configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with the base constant and other compressed media types in this file, consider using 'zstd compressed model weight configuration'.

Suggested change
// MediaTypeModelWeightConfigZstd specifies the media type for zstd compressed configuration of the model weights, including files like `tokenizer.json`, `config.json`, etc.
// MediaTypeModelWeightConfigZstd specifies the media type for a zstd compressed model weight configuration, including files like tokenizer.json, config.json, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant