Skip to content

Correct OptionEnum docblocks that describe folded-in values as snake_case - #287

Open
saarnilauri wants to merge 1 commit into
WordPress:trunkfrom
saarnilauri:fix/option-enum-value-case-comments
Open

saarnilauri wants to merge 1 commit into
WordPress:trunkfrom
saarnilauri:fix/option-enum-value-case-comments

Conversation

@saarnilauri

Copy link
Copy Markdown
Contributor

Description of the change

OptionEnum folds ModelConfig::KEY_* constants in via determineClassEnumerations(). Four comments in the file state or imply that the resulting values are snake_case. They are camelCase, and always have been:

ModelConfig::KEY_FUNCTION_DECLARATIONS = 'functionDeclarations';
ModelConfig::KEY_MAX_TOKENS            = 'maxTokens';
ModelConfig::KEY_OUTPUT_MIME_TYPE      = 'outputMimeType';

determineClassEnumerations() copies $constantValue in verbatim — there is no case transformation anywhere in the method:

if (is_string($constantValue)) {
    $constants[$enumConstantName] = $constantValue;
}

The confusion is easy to arrive at honestly, because the enum constant name genuinely is upper snake case (KEY_FUNCTION_DECLARATIONSFUNCTION_DECLARATIONS); it is only the value that is camelCase. This PR makes that name/value distinction explicit rather than leaving it implied.

Verified against trunk:

OptionEnum::functionDeclarations()  ->  functionDeclarations
OptionEnum::inputModalities()       ->  inputModalities

This is a documentation-only change. Every changed line is a comment; no constant, signature or expression is touched.

Why it matters

These comments are load-bearing for provider authors, who need the option's string value to compare against ModelMetadata::getSupportedOptions() or to inspect serialized metadata. Taking the comments at face value produces code that silently never matches.

It has already cost real debugging time twice in the Ollama provider:

  • A tool-support bug was initially reported as "the provider never declares function_declarations" (Fueled/ai-provider-for-ollama#93). The provider did declare it; a grep for the snake_case string simply could not find it. The real defect turned out to be the opposite of the one reported.
  • The fix for that issue (Fueled/ai-provider-for-ollama#94) carries a defensive option.name === 'function_declarations' branch in its admin JS, guarding against a value that no released version of this package has ever emitted.

Changes

src/Providers/Models/Enums/OptionEnum.php, four comments:

  1. Class docblock — notes that folded-in values are camelCase, not snake_case.
  2. determineClassEnumerations() docblock — replaces "converted to snake_case values" with a description that separates the constant name from the value, plus a concrete example.
  3. Inline comments in the fold loop — "the snake_case version stored in ModelConfig" / "already stores these as snake_case strings" → camelCase.
  4. INPUT_MODALITIES docblock — see below.

A related inaccuracy, and a possible follow-up

The INPUT_MODALITIES docblock claimed:

This constant is not in ModelConfig as it's derived from message content, not configured directly.

The first half is no longer true. ModelConfig does define KEY_INPUT_MODALITIES = 'inputModalities' (added deliberately, with its own note explaining it exists for model discovery). Because the fold runs after parent::determineClassEnumerations() and assigns unconditionally, the ModelConfig value overwrites the locally declared one:

OptionEnum::INPUT_MODALITIES          // 'input_modalities'  (the literal)
(string) OptionEnum::inputModalities() // 'inputModalities'   (what callers actually get)

So INPUT_MODALITIES = 'input_modalities' is effectively dead — its value never reaches a caller. I have only corrected the docblock here, so this PR stays documentation-only.

Removing the constant looks safe (nothing in src/ or tests/ references OptionEnum::INPUT_MODALITIES, and 'input_modalities' appears nowhere else in the package), and docs/ARCHITECTURE.md:175 mentions it only in prose. But that is a code change with a different review calculus, so I have left it out. Happy to open a separate PR, fold it in here, or drop the idea — whichever you prefer.

How to test the change

No behaviour to test. To confirm the values the comments now describe:

php -r 'require "vendor/autoload.php";
use WordPress\AiClient\Providers\Models\Enums\OptionEnum;
echo (string) OptionEnum::functionDeclarations(), "\n";
echo (string) OptionEnum::inputModalities(), "\n";
echo OptionEnum::INPUT_MODALITIES, "\n";'

Expected:

functionDeclarations
inputModalities
input_modalities

phpcs --standard=PSR12 is clean on the changed file, and php -l passes.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Tracing the folded-in values through git history to confirm they were never snake_case, verifying the emitted values at runtime, and drafting these comment corrections. The final wording was reviewed and edited by me.

🤖 Generated with Claude Code

…case

The values folded in from ModelConfig KEY_* constants are copied verbatim
and are camelCase (e.g. 'functionDeclarations'), not snake_case. Only the
enum constant name is upper snake case. Four comments stated or implied
otherwise, which has misled provider authors comparing option names
against ModelMetadata::getSupportedOptions().

Also corrects the INPUT_MODALITIES docblock: ModelConfig does define
KEY_INPUT_MODALITIES, and because the fold assigns unconditionally after
parent::determineClassEnumerations(), that value takes precedence over the
locally declared 'input_modalities'.

Documentation only; every changed line is a comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: saarnilauri <laurisaarni@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.54%. Comparing base (20a1a6d) to head (4896b8d).

Additional details and impacted files
@@            Coverage Diff            @@
##              trunk     #287   +/-   ##
=========================================
  Coverage     86.54%   86.54%           
  Complexity     1381     1381           
=========================================
  Files            69       69           
  Lines          4438     4438           
=========================================
  Hits           3841     3841           
  Misses          597      597           
Flag Coverage Δ
unit 86.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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