Refactored case sniffs to allow choosing snake or camel case. #10
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRenames snake_case-specific sniffs to generic Naming variants and makes naming format configurable via public Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Ruleset / Sniff config
participant SniffBase as AbstractVariableNamingSniff
participant Concrete as Local/ParameterNamingSniff
participant File as PHP file scanner
Note over Config,SniffBase: Config injects public $format (default 'snakeCase')
Config->>SniffBase: set $format
File->>Concrete: token stream (variable/parameter encountered)
Concrete->>SniffBase: isValidFormat(name)?
alt name valid for configured format
SniffBase-->>Concrete: true
Concrete->>File: no error
else name invalid for configured format
SniffBase-->>Concrete: false
Concrete->>SniffBase: toFormat(name) → suggestion
Concrete->>File: addFixableError(message, code based on $format, [suggestion])
File->>Concrete: optionally apply fix (phpcbf)
end
Note right of Concrete: Error code chosen dynamically: NotSnakeCase or NotCamelCase
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/Unit/ParameterNamingSniffTest.php (1)
119-195: Good coverage for camelCase parameter naming.The new test method properly validates camelCase behavior for parameters:
- Creates temporary ruleset with format configuration
- Tests valid and invalid cases
- Proper cleanup with try-finally
The static analysis hints (lines 128, 135, 144, 154, 157) are false positives—this codebase intentionally enforces snake_case for local variables.
Note: As mentioned in the
LocalVariableNamingSniffTest.phpreview, the temporary ruleset creation logic is duplicated. Consider extracting to a shared helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
CLAUDE.md(9 hunks)README.md(3 hunks)composer.json(1 hunks)src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php(3 hunks)src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php(2 hunks)src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php(3 hunks)src/DrevOps/ruleset.xml(1 hunks)tests/Functional/LocalVariableNamingSniffFunctionalTest.php(4 hunks)tests/Functional/ParameterNamingSniffFunctionalTest.php(5 hunks)tests/Unit/AbstractVariableNamingSniffTest.php(11 hunks)tests/Unit/LocalVariableNamingSniffTest.php(3 hunks)tests/Unit/ParameterNamingSniffTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Functional/ParameterNamingSniffFunctionalTest.php (1)
tests/Functional/FunctionalTestCase.php (1)
FunctionalTestCase(17-142)
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (1)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (3)
AbstractVariableNamingSniff(16-573)isValidFormat(102-108)toFormat(168-174)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php (1)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (4)
AbstractVariableNamingSniff(16-573)isParameter(438-472)isValidFormat(102-108)toFormat(168-174)
tests/Unit/ParameterNamingSniffTest.php (3)
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (1)
ParameterNamingSniff(17-194)tests/Unit/UnitTestCase.php (2)
UnitTestCase(18-152)processCode(46-53)src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (1)
register(50-52)
🪛 GitHub Actions: Test PHP
composer.json
[error] 1-1: ergebnis/composer-normalize detected that composer.json is not normalized and exited with code 1 during 'composer normalize --dry-run'. See diff for required changes to add license and type fields.
🪛 PHPMD (2.15.0)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php
106-106: Missing class import via use statement (line '106', column '28'). (undefined)
(MissingImport)
172-172: Missing class import via use statement (line '172', column '28'). (undefined)
(MissingImport)
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php
60-60: The variable $error_code is not named in camelCase. (undefined)
(CamelCaseVariableName)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php
63-63: The variable $error_code is not named in camelCase. (undefined)
(CamelCaseVariableName)
tests/Unit/ParameterNamingSniffTest.php
128-164: The parameter $should_have_errors is not named in camelCase. (undefined)
(CamelCaseParameterName)
135-135: The variable $ruleset_xml is not named in camelCase. (undefined)
(CamelCaseVariableName)
144-144: The variable $ruleset_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
154-154: The variable $should_have_errors is not named in camelCase. (undefined)
(CamelCaseVariableName)
157-159: The method testProcessWithCamelCaseFormat uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
tests/Unit/AbstractVariableNamingSniffTest.php
489-489: Missing class import via use statement (line '489', column '23'). (undefined)
(MissingImport)
529-529: Missing class import via use statement (line '529', column '23'). (undefined)
(MissingImport)
568-568: Missing class import via use statement (line '568', column '23'). (undefined)
(MissingImport)
606-606: Missing class import via use statement (line '606', column '23'). (undefined)
(MissingImport)
634-634: Missing class import via use statement (line '634', column '23'). (undefined)
(MissingImport)
648-648: Missing class import via use statement (line '648', column '23'). (undefined)
(MissingImport)
tests/Unit/LocalVariableNamingSniffTest.php
134-170: The parameter $should_have_errors is not named in camelCase. (undefined)
(CamelCaseParameterName)
141-141: The variable $ruleset_xml is not named in camelCase. (undefined)
(CamelCaseVariableName)
150-150: The variable $ruleset_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
160-160: The variable $should_have_errors is not named in camelCase. (undefined)
(CamelCaseVariableName)
163-165: The method testProcessWithCamelCaseFormat uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🔇 Additional comments (13)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php (1)
11-22: Functional expectations correctly aligned with new LocalVariableNamingSniffRenamed test class, updated
$sniffSource, and the expected messages/sources (... not in snakeCase format; ...,LocalVariableNaming.NotSnakeCase) all match the new configurable naming behavior and defaultsnakeCaseformat. Looks consistent with the sniff implementation.Also applies to: 34-67, 80-83, 96-103
src/DrevOps/ruleset.xml (1)
5-15: Ruleset correctly switched to generic Naming sniffs with configurable formatReferencing
DrevOps.NamingConventions.LocalVariableNamingandParameterNaming, with the commentedformat="camelCase"examples, matches the new configurable$formatproperty. Defaults and docs are consistent.README.md (1)
22-22: Documentation updates for configurable naming and error codes are consistentThe README changes describing configurable
snakeCase/camelCase, theLocalVariableNamingandParameterNamingsniffs, and the documented error codes (...NotSnakeCase/...NotCamelCase) all line up with the new sniff implementations and ruleset configuration.Also applies to: 63-65, 73-91, 93-103, 113-121, 126-126, 151-155, 159-159
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (1)
11-16: Configurable format helpers in base sniff look correct and cohesiveThe new
$formatproperty,isCamelCase(),isValidFormat(),toCamelCase(), andtoFormat()correctly centralize format handling, defaulting to'snakeCase'and supporting'camelCase'with clear exceptions for invalid values. This aligns with the updated ruleset and docs, and keeps derived sniffs simple.Also applies to: 18-25, 80-91, 93-108, 138-157, 159-174
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php (1)
10-16: Local variable sniff correctly delegates to base format logicUsing
isValidFormat()/toFormat()with$this->formatand switching betweenCODE_VARIABLE_NOT_SNAKE_CASEandCODE_VARIABLE_NOT_CAMEL_CASEgives the expected behavior for both snakeCase (default) and camelCase. Skipping parameters (handled byParameterNamingSniff), properties, and reserved/static variables keeps the scope precise.Also applies to: 23-27, 51-52, 56-66, 70-71
tests/Functional/ParameterNamingSniffFunctionalTest.php (1)
11-130: LGTM! Consistent refactoring across functional tests.The rename from
ParameterSnakeCaseSniffFunctionalTesttoParameterNamingSniffFunctionalTestis consistently applied throughout the file, including:
- Class name and documentation
- Sniff source references
- Test expectations with updated error codes
- phpcbf command configuration
- Internal comments
All test assertions correctly reference the new
ParameterNamingsniff source and maintain backward compatibility by continuing to test snake_case format (the default).src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (2)
10-27: LGTM! Proper extension for configurable format support.The docblock and class declaration correctly reflect the refactoring:
- Documentation now mentions both snakeCase and camelCase support
- New constant
CODE_PARAMETER_NOT_CAMEL_CASEadded alongside the existing snake_case constant- Extends the renamed
AbstractVariableNamingSniffbase class
54-67: LGTM! Dynamic error code selection based on configured format.The validation logic correctly:
- Uses
isValidFormat()andtoFormat()from the base class for format-agnostic checking- Dynamically determines the error code based on
$this->format- Provides clear, format-specific error messages
The static analysis hint about
$error_codenot being in camelCase is a false positive—this codebase intentionally uses snake_case for local variables, which is what this sniff enforces.tests/Unit/LocalVariableNamingSniffTest.php (1)
7-38: LGTM! Test updated for renamed sniff and new error code.The changes correctly reflect the refactoring:
- Import and class name updated to
LocalVariableNamingSniff- Configuration reference updated to
LocalVariableNaming- Test for both
CODE_VARIABLE_NOT_SNAKE_CASEandCODE_VARIABLE_NOT_CAMEL_CASEconstantstests/Unit/AbstractVariableNamingSniffTest.php (1)
7-425: LGTM! Consistent refactoring of base test class.All references to the abstract base class and concrete implementations are correctly updated:
- Imports changed to
AbstractVariableNamingSniff,LocalVariableNamingSniff,ParameterNamingSniff- Class name and documentation updated
- All test instantiations updated to use new class names
The existing test logic remains sound and comprehensive.
tests/Unit/ParameterNamingSniffTest.php (2)
7-48: LGTM! Test properly updated for renamed sniff.The changes correctly reflect the refactoring:
- Imports and class name updated to
ParameterNamingSniff- Configuration reference updated to
ParameterNaming- Test validates both
CODE_PARAMETER_NOT_SNAKE_CASEandCODE_PARAMETER_NOT_CAMEL_CASEconstants
208-208: LGTM! Instantiation updated correctly.The test properly uses the renamed
ParameterNamingSniffclass.CLAUDE.md (1)
1-296: LGTM! Comprehensive documentation updates.The documentation thoroughly reflects all the refactoring changes:
- Project overview updated to describe configurable naming (snakeCase/camelCase)
- Class and file names consistently updated throughout
- New public properties and constants documented (
$format,CODE_*_NOT_CAMEL_CASE)- New methods documented (
isCamelCase(),toCamelCase(),isValidFormat(),toFormat())- Error codes updated with both format variants
- Test file references updated
- Examples updated to reflect new naming conventions
The documentation provides clear guidance for developers working with this codebase and accurately describes the new configurability feature.
| "description": "DrevOps PHP_CodeSniffer rules: enforce consistent naming (snakeCase or camelCase) for variables and parameters, PHPUnit data provider conventions.", | ||
| "keywords": [ | ||
| "phpcs", | ||
| "phpcodesniffer", | ||
| "standards", | ||
| "snakeCase", | ||
| "camelCase", | ||
| "naming-conventions", | ||
| "code-quality", | ||
| "phpunit", | ||
| "data-providers" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
Fix composer-normalize failure by normalizing key order
The new description/keywords look good, but the pipeline failure indicates ergebnis/composer-normalize expects composer.json to be re-normalized (likely key ordering around description, type, keywords, license). Please run composer normalize locally and commit the resulting changes so Test PHP passes.
🏁 Script executed:
#!/bin/bash
# Inspect the composer.json file to understand its current structure
cat composer.json | head -30Length of output: 959
🏁 Script executed:
#!/bin/bash
# Check for any configuration files related to composer-normalize
find . -maxdepth 2 -name ".composer-normalize.json" -o -name ".composer-normalize" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any test or pipeline configuration that shows composer-normalize failure
find . -maxdepth 2 -type f \( -name "*.yml" -o -name "*.yaml" \) | head -10Length of output: 51
🌐 Web query:
ergebnis composer-normalize canonical key order composer.json
💡 Result:
Kurz: ergebnis/composer-normalize stellt die Schlüsselreihenfolge von composer.json nach dem offiziellen Composer‑JSON‑Schema (https://getcomposer.org/schema.json) her und wendet zusätzlich Spezial-Sortierungen an (z. B. bin, config/extra/scripts-descriptions, conflict/provide/replace/require/require-dev/suggest) sowie Normalisierung von Versionsconstraints. [1][2][3]
Quellen:
- ergebnis/composer-normalize — Beschreibung der Normalisierungen (Sortierung/Normalisierung der genannten Abschnitte). [1]
- Composer‑Dokumentation: composer.json‑Schema (canonical Struktur / schema.json). [2]
- ergebnis/composer-json-normalizer / json-normalizer — erklärt, dass die Composer‑Normalizer das Composer‑Schema zur Umstrukturierung verwenden. [3]
🌐 Web query:
Composer JSON schema key order name description type keywords license
💡 Result:
Composer itself treats JSON keys as unordered, but community tools and the official docs show a conventional, recommended ordering. A commonly used order (used by composer.json normalizers) is:
- name
- type
- description
- keywords
- homepage
- license
- authors / support / funding / time
- require
- require-dev
- conflict / replace / provide / suggest
- autoload / autoload-dev / include-path / scripts / bin
- extra / archives / repositories
- config / minimum-stability / prefer-stable / prefer-lowest / platform / platform-dev
Sources: composer official schema/docs (schema.json + 04-schema.md) and composer-normalize examples. [1][2]
References
[1] Composer schema / docs. [2] localheinz / composer-normalize examples.
Normalize composer.json key ordering to fix pipeline failure
The key ordering issue is confirmed. The current order (description → keywords → license → type) violates the canonical Composer schema, which expects: name → type → description → keywords → homepage → license. Running composer normalize locally will reorder these keys correctly and resolve the pipeline failure.
🤖 Prompt for AI Agents
In composer.json around lines 3 to 14 the top-level keys are out of the
canonical Composer order (description → keywords → license → type) which causes
the pipeline failure; run `composer normalize` locally or reorder the keys to
the expected sequence (name → type → description → keywords → homepage →
license) so the file matches Composer's canonical schema and commits the
normalized composer.json to fix the pipeline.
| /** | ||
| * Test camelCase detection. | ||
| * | ||
| * @param string $name | ||
| * The variable name to test. | ||
| * @param bool $expected | ||
| * Expected result. | ||
| */ | ||
| #[DataProvider('dataProviderCamelCaseDetection')] | ||
| public function testCamelCaseDetection(string $name, bool $expected): void { | ||
| $sniff = new LocalVariableNamingSniff(); | ||
| $reflection = new \ReflectionClass($sniff); | ||
| $method = $reflection->getMethod('isCamelCase'); | ||
|
|
||
| $result = $method->invoke($sniff, $name); | ||
| $this->assertSame($expected, $result, 'Failed for: ' . $name); | ||
| } | ||
|
|
||
| /** | ||
| * Data provider for camelCase detection tests. | ||
| * | ||
| * @return array<string, array<string|bool>> | ||
| * Test cases. | ||
| */ | ||
| public static function dataProviderCamelCaseDetection(): array { | ||
| return [ | ||
| 'valid_single_word' => ['test', TRUE], | ||
| 'valid_camelCase' => ['testVariable', TRUE], | ||
| 'valid_with_number' => ['test123', TRUE], | ||
| 'valid_camelCase_with_number' => ['testVariable123', TRUE], | ||
| 'valid_long_camelCase' => ['testLongVariableName', TRUE], | ||
| 'invalid_snake_case' => ['test_variable', FALSE], | ||
| 'invalid_PascalCase' => ['TestVariable', FALSE], | ||
| 'invalid_uppercase' => ['TEST', FALSE], | ||
| 'invalid_starting_uppercase' => ['Test', FALSE], | ||
| 'invalid_with_underscore' => ['test_var', FALSE], | ||
| 'invalid_leading_underscore' => ['_test', FALSE], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Test camelCase conversion. | ||
| * | ||
| * @param string $input | ||
| * The input name. | ||
| * @param string $expected | ||
| * Expected output. | ||
| */ | ||
| #[DataProvider('providerToCamelCase')] | ||
| public function testToCamelCase(string $input, string $expected): void { | ||
| $sniff = new LocalVariableNamingSniff(); | ||
| $reflection = new \ReflectionClass($sniff); | ||
| $method = $reflection->getMethod('toCamelCase'); | ||
|
|
||
| $result = $method->invoke($sniff, $input); | ||
| $this->assertSame($expected, $result); | ||
| } | ||
|
|
||
| /** | ||
| * Data provider for toCamelCase conversion tests. | ||
| * | ||
| * @return array<string, array<string>> | ||
| * Test cases. | ||
| */ | ||
| public static function providerToCamelCase(): array { | ||
| return [ | ||
| 'snake_case' => ['test_variable', 'testVariable'], | ||
| 'PascalCase' => ['TestVariable', 'testvariable'], | ||
| 'already_camel' => ['testVariable', 'testvariable'], | ||
| 'with_numbers' => ['test_123_variable', 'test123Variable'], | ||
| 'multiple_underscores' => ['test__variable', 'testVariable'], | ||
| 'leading_underscore' => ['_test_variable', 'testVariable'], | ||
| 'single_word' => ['test', 'test'], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Test isValidFormat() method. | ||
| * | ||
| * @param string $format | ||
| * The format to configure. | ||
| * @param string $name | ||
| * The variable name to test. | ||
| * @param bool $expected | ||
| * Expected result. | ||
| */ | ||
| #[DataProvider('providerIsValidFormat')] | ||
| public function testIsValidFormat(string $format, string $name, bool $expected): void { | ||
| $sniff = new LocalVariableNamingSniff(); | ||
| $sniff->format = $format; | ||
| $reflection = new \ReflectionClass($sniff); | ||
| $method = $reflection->getMethod('isValidFormat'); | ||
|
|
||
| $result = $method->invoke($sniff, $name); | ||
| $this->assertSame($expected, $result); | ||
| } | ||
|
|
||
| /** | ||
| * Data provider for isValidFormat tests. | ||
| * | ||
| * @return array<string, array<string|bool>> | ||
| * Test cases. | ||
| */ | ||
| public static function providerIsValidFormat(): array { | ||
| return [ | ||
| 'snakeCase_valid_snake' => ['snakeCase', 'test_variable', TRUE], | ||
| 'snakeCase_invalid_camel' => ['snakeCase', 'testVariable', FALSE], | ||
| 'camelCase_valid_camel' => ['camelCase', 'testVariable', TRUE], | ||
| 'camelCase_invalid_snake' => ['camelCase', 'test_variable', FALSE], | ||
| 'snakeCase_single_word' => ['snakeCase', 'test', TRUE], | ||
| 'camelCase_single_word' => ['camelCase', 'test', TRUE], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Test toFormat() method. | ||
| * | ||
| * @param string $format | ||
| * The format to configure. | ||
| * @param string $input | ||
| * The input name. | ||
| * @param string $expected | ||
| * Expected output. | ||
| */ | ||
| #[DataProvider('providerToFormat')] | ||
| public function testToFormat(string $format, string $input, string $expected): void { | ||
| $sniff = new LocalVariableNamingSniff(); | ||
| $sniff->format = $format; | ||
| $reflection = new \ReflectionClass($sniff); | ||
| $method = $reflection->getMethod('toFormat'); | ||
|
|
||
| $result = $method->invoke($sniff, $input); | ||
| $this->assertSame($expected, $result); | ||
| } | ||
|
|
||
| /** | ||
| * Data provider for toFormat tests. | ||
| * | ||
| * @return array<string, array<string>> | ||
| * Test cases. | ||
| */ | ||
| public static function providerToFormat(): array { | ||
| return [ | ||
| 'snakeCase_from_camel' => ['snakeCase', 'testVariable', 'test_variable'], | ||
| 'snakeCase_from_snake' => ['snakeCase', 'test_variable', 'test_variable'], | ||
| 'camelCase_from_snake' => ['camelCase', 'test_variable', 'testVariable'], | ||
| 'camelCase_from_camel' => ['camelCase', 'testVariable', 'testvariable'], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Test that isValidFormat() throws exception for invalid format. | ||
| */ | ||
| public function testIsValidFormatThrowsExceptionForInvalidFormat(): void { | ||
| $sniff = new LocalVariableNamingSniff(); | ||
| $sniff->format = 'invalidFormat'; | ||
| $reflection = new \ReflectionClass($sniff); | ||
| $method = $reflection->getMethod('isValidFormat'); | ||
|
|
||
| $this->expectException(\RuntimeException::class); | ||
| $this->expectExceptionMessage('Invalid format: invalidFormat'); | ||
| $method->invoke($sniff, 'test'); | ||
| } | ||
|
|
||
| /** | ||
| * Test that toFormat() throws exception for invalid format. | ||
| */ | ||
| public function testToFormatThrowsExceptionForInvalidFormat(): void { | ||
| $sniff = new LocalVariableNamingSniff(); | ||
| $sniff->format = 'invalidFormat'; | ||
| $reflection = new \ReflectionClass($sniff); | ||
| $method = $reflection->getMethod('toFormat'); | ||
|
|
||
| $this->expectException(\RuntimeException::class); | ||
| $this->expectExceptionMessage('Invalid format: invalidFormat'); | ||
| $method->invoke($sniff, 'test'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Comprehensive test coverage for camelCase support.
The new tests thoroughly validate:
- camelCase detection with various edge cases
- camelCase conversion from different formats
- Format-agnostic validation via
isValidFormat() - Format-agnostic conversion via
toFormat() - Exception handling for invalid format strings
The static analysis hints about missing \ReflectionClass imports (lines 489, 529, 568, 606, 634, 648) are false positives—it's a built-in PHP class.
Verify the toCamelCase conversion behavior.
The test data at lines 545-546 and 624 suggests that converting 'TestVariable' to camelCase produces 'testvariable' (all lowercase). This seems unusual—typically, 'TestVariable' (PascalCase) would become 'testVariable' (camelCase). Please verify this is the intended behavior.
🏁 Script executed:
#!/bin/bash
# Description: Check the toCamelCase implementation to verify conversion logic
# Find and display the toCamelCase method implementation
ast-grep --pattern $'protected function toCamelCase(string $name): string {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find LocalVariableNamingSniff class file
fd -t f "LocalVariableNamingSniff" --extension phpLength of output: 169
🏁 Script executed:
#!/bin/bash
# Search for toCamelCase method definition across codebase
rg -n "function toCamelCase" --type phpLength of output: 129
🏁 Script executed:
#!/bin/bash
# Search for the method with broader pattern
rg -n "toCamelCase" --type php -B 2 -A 10 | head -50Length of output: 4433
🏁 Script executed:
#!/bin/bash
# Check functional tests for expected behavior of toCamelCase
grep -n "toCamelCase\|testVariable\|TestVariable" tests/Functional/LocalVariableNamingSniffFunctionalTest.php | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation explaining the toCamelCase behavior
grep -B 5 -A 15 "protected function toCamelCase" src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.phpLength of output: 579
Critical bug in toCamelCase implementation and test validation.
The toCamelCase() method in src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (lines 147-156) has a fundamental flaw: it only handles underscore-separated strings. For inputs without underscores (PascalCase, camelCase), it simply lowercases the entire string, which is incorrect.
Example:
- Input:
'TestVariable'→ Output:'testvariable'❌ (should be'testVariable') - Input:
'testVariable'→ Output:'testvariable'❌ (should be'testVariable') - Input:
'test_variable'→ Output:'testVariable'✓ (correct)
The test data at tests/Unit/AbstractVariableNamingSniffTest.php (lines 545-546) validates this broken behavior rather than detecting it as incorrect. The method must be fixed to properly handle non-underscore-separated inputs before these tests can be considered valid.
🧰 Tools
🪛 PHPMD (2.15.0)
489-489: Missing class import via use statement (line '489', column '23'). (undefined)
(MissingImport)
529-529: Missing class import via use statement (line '529', column '23'). (undefined)
(MissingImport)
568-568: Missing class import via use statement (line '568', column '23'). (undefined)
(MissingImport)
606-606: Missing class import via use statement (line '606', column '23'). (undefined)
(MissingImport)
634-634: Missing class import via use statement (line '634', column '23'). (undefined)
(MissingImport)
648-648: Missing class import via use statement (line '648', column '23'). (undefined)
(MissingImport)
🤖 Prompt for AI Agents
In src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php around
lines 147-156 and tests/Unit/AbstractVariableNamingSniffTest.php around lines
545-546, toCamelCase currently lowercases the entire input when there are no
underscores; change it so underscore-handling remains (split on '_' and camelize
following segments), but for inputs without underscores: if the first character
is uppercase (PascalCase) convert only the first character to lowercase to
produce camelCase, if the first character is already lowercase (already
camelCase or single word) return the input unchanged; preserve digits and
internal uppercase letters. After fixing the method, update the test
expectations in tests/Unit/AbstractVariableNamingSniffTest.php so 'PascalCase'
and 'already_camel' (and any related assertions) expect 'testVariable' (not
'testvariable'), and update any other expectations that assumed the old broken
behavior.
| /** | ||
| * Test process method with camelCase format. | ||
| * | ||
| * @param string $code | ||
| * PHP code to test. | ||
| * @param bool $should_have_errors | ||
| * Whether errors should be detected. | ||
| */ | ||
| #[DataProvider('dataProviderProcessWithCamelCaseFormat')] | ||
| public function testProcessWithCamelCaseFormat(string $code, bool $should_have_errors): void { | ||
| // Create a new config with camelCase format property. | ||
| $config = new Config(); | ||
| $config->standards = ['DrevOps']; | ||
| $config->sniffs = ['DrevOps.NamingConventions.LocalVariableNaming']; | ||
|
|
||
| // Create temporary ruleset XML with property configuration. | ||
| $ruleset_xml = '<?xml version="1.0"?> | ||
| <ruleset name="Test"> | ||
| <rule ref="DrevOps.NamingConventions.LocalVariableNaming"> | ||
| <properties> | ||
| <property name="format" value="camelCase"/> | ||
| </properties> | ||
| </rule> | ||
| </ruleset>'; | ||
|
|
||
| $ruleset_file = tempnam(sys_get_temp_dir(), 'phpcs_ruleset_'); | ||
| file_put_contents($ruleset_file, $ruleset_xml); | ||
|
|
||
| try { | ||
| $config->standards = [$ruleset_file]; | ||
| $this->ruleset = new Ruleset($config); | ||
|
|
||
| $file = $this->processCode($code); | ||
| $errors = $file->getErrors(); | ||
|
|
||
| if ($should_have_errors) { | ||
| $this->assertNotEmpty($errors); | ||
| } | ||
| else { | ||
| $this->assertEmpty($errors); | ||
| } | ||
| } | ||
| finally { | ||
| unlink($ruleset_file); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Data provider for camelCase format tests. | ||
| * | ||
| * @return array<string, array<mixed>> | ||
| * Test cases. | ||
| */ | ||
| public static function dataProviderProcessWithCamelCaseFormat(): array { | ||
| return [ | ||
| 'valid_camel_case_variable' => [ | ||
| '<?php $validVariable = 1;', | ||
| FALSE, | ||
| ], | ||
| 'invalid_snake_case_variable' => [ | ||
| '<?php $invalid_variable = 1;', | ||
| TRUE, | ||
| ], | ||
| 'single_word_valid' => [ | ||
| '<?php $test = 1;', | ||
| FALSE, | ||
| ], | ||
| 'local_variable_in_method_valid' => [ | ||
| '<?php class Test { public function test() { $validVar = 1; } }', | ||
| FALSE, | ||
| ], | ||
| 'local_variable_in_method_invalid' => [ | ||
| '<?php class Test { public function test() { $invalid_var = 1; } }', | ||
| TRUE, | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good addition of camelCase format tests.
The new test method testProcessWithCamelCaseFormat properly validates camelCase behavior:
- Creates a temporary ruleset XML with format configuration
- Uses try-finally for proper cleanup
- Data provider covers valid camelCase, invalid snake_case, and edge cases
The static analysis hints (lines 134, 141, 150, 160, 163) about variable naming and else expressions are false positives—this codebase intentionally uses snake_case for local variables.
Note: This test setup pattern (creating temporary ruleset XML) appears to be duplicated in ParameterNamingSniffTest.php. Consider extracting this into a shared helper method in UnitTestCase for maintainability.
🧰 Tools
🪛 PHPMD (2.15.0)
134-170: The parameter $should_have_errors is not named in camelCase. (undefined)
(CamelCaseParameterName)
141-141: The variable $ruleset_xml is not named in camelCase. (undefined)
(CamelCaseVariableName)
150-150: The variable $ruleset_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
160-160: The variable $should_have_errors is not named in camelCase. (undefined)
(CamelCaseVariableName)
163-165: The method testProcessWithCamelCaseFormat uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
In tests/Unit/LocalVariableNamingSniffTest.php around lines 125 to 201, the test
duplicates temporary ruleset XML creation and triggers static analysis
false-positives for snake_case locals; extract the temp ruleset creation and
cleanup into a protected helper on UnitTestCase (e.g.
createTemporaryRuleset(string $xml): string returning filepath and handling
unlink in tearDown or via returned disposable), update this test to call that
helper, and apply the same refactor in ParameterNamingSniffTest.php;
additionally, where local snake_case variables are intentionally used, add the
appropriate static-analysis/PHPMD/PHPCS ignore annotations or inline suppression
comments so the analyzer no longer raises false positives.
AlexSkrypnyk
force-pushed
the
feature/allow-chose-case
branch
from
c594a2f to
4f2ec80
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
===========================================
- Coverage 100.00% 99.51% -0.49%
===========================================
Files 6 6
Lines 387 409 +22
===========================================
+ Hits 387 407 +20
- Misses 0 2 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
180-225: Testing documentation updated correctly.The testing strategy section has been updated with the new class and file names. The content accurately describes the test organization.
Note: Lines 186, 191, 205, and 210 use bold emphasis (
**...**) where markdown headings (###) might be more appropriate for document structure. This is a minor formatting consideration.
♻️ Duplicate comments (5)
composer.json (1)
3-16: Description/keywords match new naming features; re-runcomposer normalizefor key orderThe updated description and keywords accurately reflect the configurable snakeCase/camelCase naming and PHPUnit data-provider focus. However, the top-level key order (description/license/type/keywords/homepage) still looks non-canonical for
ergebnis/composer-normalize, which previously caused pipeline failures. Please re-runcomposer normalizeand commit the normalizedcomposer.jsonso CI passes.README.md (1)
134-144: MakeParameterNamingexamples fully valid PHP functionsThe
ParameterNamingexamples still show two barefunction processOrder(...)lines in a single fenced block without bodies or closing braces, which can be confusing. Converting them into full functions (as previously suggested) will make the examples clearer and syntactically valid.Apply something along these lines:
-**With `snakeCase` (default):** -```php -function processOrder($order_id, $user_data) { // ✓ Valid -function processOrder($orderId, $userData) { // ✗ Error: NotSnakeCase -``` +**With `snakeCase` (default):** +```php +function processOrder($order_id, $user_data) { // ✓ Valid + // ... +} + +function processOrder($orderId, $userData) { // ✗ Error: NotSnakeCase + // ... +} +``` @@ -**With `camelCase`:** -```php -function processOrder($orderId, $userData) { // ✓ Valid -function processOrder($order_id, $user_data) { // ✗ Error: NotCamelCase -``` +**With `camelCase`:** +```php +function processOrder($orderId, $userData) { // ✓ Valid + // ... +} + +function processOrder($order_id, $user_data) { // ✗ Error: NotCamelCase + // ... +} +```This keeps the focus on parameter naming while giving readers complete, copy-pasteable PHP examples.
tests/Unit/LocalVariableNamingSniffTest.php (1)
125-201: Fix camelCase-format unit test wiring so the custom ruleset actually takes effectThe failures in
testProcessWithCamelCaseFormatcome from constructing a newConfigthat is never stored on the test instance, whileprocessCode()uses$this->configto build theRuleset, so theformat="camelCase"ruleset is never applied (the sniff stays in defaultsnakeCasemode, flipping your expectations). Assign the new config back to$this->configbefore creating the ruleset. For example:try { - $config->standards = [$ruleset_file]; - $this->ruleset = new Ruleset($config); + $config->standards = [$ruleset_file]; + // Use this config for subsequent processCode() calls. + $this->config = $config; + $this->ruleset = new Ruleset($this->config);With this change, the camelCase data provider cases should match the expected presence/absence of errors.
tests/Unit/AbstractVariableNamingSniffTest.php (2)
542-552: Critical bug in test expectations - previously flagged.Lines 545-546 validate incorrect behavior where
'TestVariable'converts to'testvariable'and'testVariable'converts to'testvariable'. This issue was already identified in previous review comments.As noted in the past review:
- The
toCamelCase()implementation only handles underscore-separated strings correctly- For inputs without underscores, it incorrectly lowercases the entire string
- Expected:
'TestVariable'→'testVariable','testVariable'→'testVariable'- Actual: Both convert to
'testvariable'
619-626: Test data validates broken toCamelCase behavior - previously flagged.Line 624 expects
'testVariable'to convert to'testvariable'when the format is'camelCase', which is incorrect. This is the same underlyingtoCamelCase()bug already identified in previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
CLAUDE.md(9 hunks)README.md(3 hunks)composer.json(1 hunks)src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php(3 hunks)src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php(2 hunks)src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php(3 hunks)src/DrevOps/ruleset.xml(1 hunks)tests/Functional/LocalVariableNamingSniffFunctionalTest.php(4 hunks)tests/Functional/ParameterNamingSniffFunctionalTest.php(5 hunks)tests/Unit/AbstractVariableNamingSniffTest.php(11 hunks)tests/Unit/LocalVariableNamingSniffTest.php(2 hunks)tests/Unit/ParameterNamingSniffTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php (1)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (3)
AbstractVariableNamingSniff(16-573)isValidFormat(102-108)toFormat(168-174)
tests/Functional/ParameterNamingSniffFunctionalTest.php (2)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php (1)
CoversNothing(16-109)tests/Functional/FunctionalTestCase.php (1)
FunctionalTestCase(17-142)
tests/Unit/ParameterNamingSniffTest.php (3)
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (1)
ParameterNamingSniff(17-194)tests/Unit/UnitTestCase.php (2)
UnitTestCase(18-152)processCode(46-53)src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (1)
register(50-52)
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (1)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (3)
AbstractVariableNamingSniff(16-573)isValidFormat(102-108)toFormat(168-174)
tests/Unit/AbstractVariableNamingSniffTest.php (3)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (1)
AbstractVariableNamingSniff(16-573)src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php (1)
LocalVariableNamingSniff(16-84)src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (1)
ParameterNamingSniff(17-194)
🪛 GitHub Actions: Test PHP
tests/Unit/LocalVariableNamingSniffTest.php
[error] 164-164: testProcessWithCamelCaseFormat@valid_camel_case_variable: Failed asserting that an array is empty.
[error] 161-161: testProcessWithCamelCaseFormat@invalid_snake_case_variable: Failed asserting that an array is not empty.
[error] 164-164: testProcessWithCamelCaseFormat@local_variable_in_method_valid: Failed asserting that an array is empty.
[error] 161-161: testProcessWithCamelCaseFormat@local_variable_in_method_invalid: Failed asserting that an array is not empty.
tests/Unit/ParameterNamingSniffTest.php
[error] 158-158: testProcessWithCamelCaseFormat@valid_camel_case_parameter: Failed asserting that an array is empty.
[error] 155-155: testProcessWithCamelCaseFormat@invalid_snake_case_parameter: Failed asserting that an array is not empty.
[error] 158-158: testProcessWithCamelCaseFormat@method_parameter_valid: Failed asserting that an array is empty.
[error] 155-155: testProcessWithCamelCaseFormat@method_parameter_invalid: Failed asserting that an array is not empty.
🪛 markdownlint-cli2 (0.18.1)
README.md
64-64: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
73-73: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
75-75: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
80-80: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
CLAUDE.md
186-186: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
191-191: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
205-205: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
210-210: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
tests/Unit/LocalVariableNamingSniffTest.php
134-170: The parameter $should_have_errors is not named in camelCase. (undefined)
(CamelCaseParameterName)
141-141: The variable $ruleset_xml is not named in camelCase. (undefined)
(CamelCaseVariableName)
150-150: The variable $ruleset_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
160-160: The variable $should_have_errors is not named in camelCase. (undefined)
(CamelCaseVariableName)
163-165: The method testProcessWithCamelCaseFormat uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php
63-63: The variable $error_code is not named in camelCase. (undefined)
(CamelCaseVariableName)
tests/Unit/ParameterNamingSniffTest.php
128-164: The parameter $should_have_errors is not named in camelCase. (undefined)
(CamelCaseParameterName)
135-135: The variable $ruleset_xml is not named in camelCase. (undefined)
(CamelCaseVariableName)
144-144: The variable $ruleset_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
154-154: The variable $should_have_errors is not named in camelCase. (undefined)
(CamelCaseVariableName)
157-159: The method testProcessWithCamelCaseFormat uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php
60-60: The variable $error_code is not named in camelCase. (undefined)
(CamelCaseVariableName)
src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php
106-106: Missing class import via use statement (line '106', column '28'). (undefined)
(MissingImport)
172-172: Missing class import via use statement (line '172', column '28'). (undefined)
(MissingImport)
tests/Unit/AbstractVariableNamingSniffTest.php
489-489: Missing class import via use statement (line '489', column '23'). (undefined)
(MissingImport)
529-529: Missing class import via use statement (line '529', column '23'). (undefined)
(MissingImport)
568-568: Missing class import via use statement (line '568', column '23'). (undefined)
(MissingImport)
606-606: Missing class import via use statement (line '606', column '23'). (undefined)
(MissingImport)
634-634: Missing class import via use statement (line '634', column '23'). (undefined)
(MissingImport)
648-648: Missing class import via use statement (line '648', column '23'). (undefined)
(MissingImport)
🔇 Additional comments (17)
src/DrevOps/ruleset.xml (1)
5-15: Ruleset rename and configurable format usage look correctThe rules now correctly reference
LocalVariableNaming/ParameterNaming, and the commentedformat="camelCase"properties accurately document how to switch formats while keeping snakeCase as the default. No issues from the ruleset side.src/DrevOps/Sniffs/NamingConventions/AbstractVariableNamingSniff.php (1)
18-25: Format configuration and helper methods are solidThe
$formatproperty plusisCamelCase(),isValidFormat(),toCamelCase(), andtoFormat()give a clear, centralized API for snakeCase/camelCase validation and suggestions, and failing fast with aRuntimeExceptionon unknown formats is an appropriate safeguard against misconfigured rulesets. This abstraction should make the derived sniffs much easier to maintain.Also applies to: 80-108, 138-174
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php (1)
9-16: Local variable sniff correctly delegates and adapts to configured formatThe refactored sniff cleanly excludes reserved vars, static property access, class/trait properties, and parameters (now delegated to
ParameterNamingSniff), then usesisValidFormat()/toFormat()and a format-dependent error code (NotSnakeCasevsNotCamelCase) to report and fix violations. This matches the documented behavior and keeps local-variable checks format-agnostic.Also applies to: 23-27, 50-52, 56-66, 70-71
README.md (1)
22-23: Docs accurately describe configurable naming and new sniffsThe README updates (top-level bullet, ruleset snippet, “Configure naming convention” XML, and the
LocalVariableNaming/ParameterNamingsections with error-code and ignore examples) all line up with the new configurableformatbehavior and the renamed sniffs. The error-code strings match the constants used in the sniffs and the PHPCS naming pattern, so users should be able to configure and suppress these checks reliably.Also applies to: 60-71, 73-91, 93-123, 126-126, 151-155, 159-161
src/DrevOps/Sniffs/NamingConventions/ParameterNamingSniff.php (1)
10-28: ParameterNamingSniff’s format-agnostic logic and error codes look correctSwitching to
isValidFormat()/toFormat()with a format-dependent error code cleanly supports bothsnakeCaseandcamelCasewithout breaking existingNotSnakeCasebehavior. The newCODE_PARAMETER_NOT_CAMEL_CASEconstant and updated message ("not in %s format") are consistent with the rest of the naming sniffs, and the parameter-only scope is clearly documented and enforced.Also applies to: 42-43, 53-62, 67-67
tests/Unit/LocalVariableNamingSniffTest.php (1)
7-10: Unit wiring and error-code assertions for LocalVariableNamingSniff are consistentThe updated imports,
CoversClassattribute, class name, and error-code checks forCODE_VARIABLE_NOT_SNAKE_CASE/CODE_VARIABLE_NOT_CAMEL_CASEare all aligned with the refactored sniff hierarchy and constants.Also applies to: 14-20, 28-31, 34-39
tests/Unit/ParameterNamingSniffTest.php (1)
7-10: ParameterNamingSniff unit wiring and error-code checks look goodThe updated imports,
CoversClass, class name,setUp()configuration,testRegister(), and the assertions for bothCODE_PARAMETER_NOT_SNAKE_CASEandCODE_PARAMETER_NOT_CAMEL_CASEare all consistent with the new configurable ParameterNamingSniff.Also applies to: 14-20, 28-31, 37-49
tests/Unit/AbstractVariableNamingSniffTest.php (5)
7-20: LGTM - Class and import renames are consistent.The imports and class declaration have been correctly updated to reflect the new naming scheme (
AbstractVariableNamingSniff,LocalVariableNamingSniff,ParameterNamingSniff).
31-476: LGTM - Test instances updated correctly.All test methods have been updated to instantiate the renamed classes (
LocalVariableNamingSniffandParameterNamingSniff). The test logic remains unchanged, which is appropriate for this refactoring.
478-516: LGTM - Comprehensive camelCase detection tests.The new
testCamelCaseDetectiontest and data provider follow the same pattern as the snake_case tests and cover appropriate edge cases (single words, numbers, invalid PascalCase, invalid snake_case, etc.).Note: The static analysis hint about missing
\ReflectionClassimport at line 489 is a false positive—it's a built-in PHP class.
554-590: LGTM - Format validation tests are well-structured.The
testIsValidFormattest properly validates that theisValidFormat()method respects the configured format setting and correctly identifies valid/invalid names for bothsnakeCaseandcamelCaseformats.Note: The static analysis hint about missing
\ReflectionClassimport at line 568 is a false positive.
628-654: LGTM - Exception handling tests are appropriate.The tests properly validate that
isValidFormat()andtoFormat()throwRuntimeExceptionwhen configured with an invalid format string. This is good defensive programming.Note: The static analysis hints about missing
\ReflectionClassimports at lines 634 and 648 are false positives.CLAUDE.md (5)
1-16: LGTM - Project overview updated accurately.The project overview has been correctly updated to reflect the configurable naming convention feature, mentioning support for both snakeCase and camelCase formats.
52-98: LGTM - Test commands and architecture references updated.All references to test files and classes have been correctly updated to use the new naming scheme (
LocalVariableNamingSniff,ParameterNamingSniff, etc.).
109-134: LGTM - AbstractVariableNamingSniff documentation is accurate.The documentation correctly describes the new
$formatproperty and the expanded API includingisCamelCase(),isValidFormat(),toCamelCase(), andtoFormat()methods.
135-162: LGTM - Sniff documentation reflects new error codes.The documentation correctly describes the new error codes for both sniffs:
LocalVariableNaming.NotSnakeCase/NotCamelCaseParameterNaming.NotSnakeCase/NotCamelCaseThis accurately reflects the format-based error reporting.
250-273: LGTM - Development guidelines updated with correct examples.The sniff development guidelines have been updated with accurate error code examples showing both
NotSnakeCaseandNotCamelCasevariants for the new naming sniffs.
| * Functional integration test for LocalVariableNamingSniff. | ||
| * | ||
| * This tests the sniff by actually running phpcs as an external command, | ||
| * which is the most reliable way to test PHPCS sniffs. | ||
| */ | ||
| #[CoversNothing] | ||
| class LocalVariableSnakeCaseSniffFunctionalTest extends FunctionalTestCase { | ||
| class LocalVariableNamingSniffFunctionalTest extends FunctionalTestCase { | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected string $sniffSource = 'DrevOps.NamingConventions.LocalVariableSnakeCase'; | ||
| protected string $sniffSource = 'DrevOps.NamingConventions.LocalVariableNaming'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LocalVariableNaming functional tests correctly target the renamed sniff and error source
The updated docblock, class name, $sniffSource, messages, and LocalVariableNaming.NotSnakeCase sources are all consistent with the new sniff and its default snakeCase format; no issues spotted. If you want full end‑to‑end coverage of the new configurability, you could later add a functional test that runs the sniff with format="camelCase" as well, similar to the new unit tests.
Also applies to: 34-66, 80-82, 96-103
🤖 Prompt for AI Agents
tests/Functional/LocalVariableNamingSniffFunctionalTest.php lines 11-22: No code
changes required — the functional test already targets the renamed sniff and
correct error source; keep as-is. If you want broader coverage, add a second
functional test that runs the sniff with format="camelCase" (mirroring the new
unit test) to verify configurability end-to-end.
| * Functional integration test for ParameterNamingSniff. | ||
| * | ||
| * This tests the sniff by actually running phpcs as an external command, | ||
| * which is the most reliable way to test PHPCS sniffs. | ||
| */ | ||
| #[CoversNothing] | ||
| class ParameterSnakeCaseSniffFunctionalTest extends FunctionalTestCase { | ||
| class ParameterNamingSniffFunctionalTest extends FunctionalTestCase { | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected string $sniffSource = 'DrevOps.NamingConventions.ParameterSnakeCase'; | ||
| protected string $sniffSource = 'DrevOps.NamingConventions.ParameterNaming'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ParameterNaming functional tests now correctly exercise the renamed sniff
The class name, $sniffSource, expected messages/sources, and phpcbf --sniffs= argument all consistently use ParameterNaming and NotSnakeCase, and the inline comment correctly refers to LocalVariableNamingSniff for body variables. As with local variables, you might later add a functional case that runs the sniff with format="camelCase" to mirror the new unit coverage, but the current updates themselves are solid.
Also applies to: 34-46, 60-67, 100-101, 118-118
🤖 Prompt for AI Agents
In tests/Functional/ParameterNamingSniffFunctionalTest.php around lines 11 to 22
(and also apply same changes to lines 34-46, 60-67, 100-101, 118), update all
occurrences so the functional test uses the renamed sniff: ensure the class name
and protected $sniffSource use 'ParameterNaming'
(DrevOps.NamingConventions.ParameterNaming), change expected message/source
identifiers to use 'NotSnakeCase', update any phpcbf/PHPCS command --sniffs=
arguments to reference ParameterNaming/NotSnakeCase, and keep the inline comment
referring to LocalVariableNamingSniff for body variables; make these
replacements consistently across the listed line ranges.
| /** | ||
| * Test process method with camelCase format. | ||
| * | ||
| * @param string $code | ||
| * PHP code to test. | ||
| * @param bool $should_have_errors | ||
| * Whether errors should be detected. | ||
| */ | ||
| #[DataProvider('dataProviderProcessWithCamelCaseFormat')] | ||
| public function testProcessWithCamelCaseFormat(string $code, bool $should_have_errors): void { | ||
| // Create a new config with camelCase format property. | ||
| $config = new Config(); | ||
| $config->standards = ['DrevOps']; | ||
| $config->sniffs = ['DrevOps.NamingConventions.ParameterNaming']; | ||
|
|
||
| // Create temporary ruleset XML with property configuration. | ||
| $ruleset_xml = '<?xml version="1.0"?> | ||
| <ruleset name="Test"> | ||
| <rule ref="DrevOps.NamingConventions.ParameterNaming"> | ||
| <properties> | ||
| <property name="format" value="camelCase"/> | ||
| </properties> | ||
| </rule> | ||
| </ruleset>'; | ||
|
|
||
| $ruleset_file = tempnam(sys_get_temp_dir(), 'phpcs_ruleset_'); | ||
| file_put_contents($ruleset_file, $ruleset_xml); | ||
|
|
||
| try { | ||
| $config->standards = [$ruleset_file]; | ||
| $this->ruleset = new Ruleset($config); | ||
|
|
||
| $file = $this->processCode($code); | ||
| $errors = $file->getErrors(); | ||
|
|
||
| if ($should_have_errors) { | ||
| $this->assertNotEmpty($errors); | ||
| } | ||
| else { | ||
| $this->assertEmpty($errors); | ||
| } | ||
| } | ||
| finally { | ||
| unlink($ruleset_file); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Data provider for camelCase format tests. | ||
| * | ||
| * @return array<string, array<mixed>> | ||
| * Test cases. | ||
| */ | ||
| public static function dataProviderProcessWithCamelCaseFormat(): array { | ||
| return [ | ||
| 'valid_camel_case_parameter' => [ | ||
| '<?php function test($validParameter) {}', | ||
| FALSE, | ||
| ], | ||
| 'invalid_snake_case_parameter' => [ | ||
| '<?php function test($invalid_parameter) {}', | ||
| TRUE, | ||
| ], | ||
| 'single_word_valid' => [ | ||
| '<?php function test($param) {}', | ||
| FALSE, | ||
| ], | ||
| 'method_parameter_valid' => [ | ||
| '<?php class Test { public function test($validParam) {} }', | ||
| FALSE, | ||
| ], | ||
| 'method_parameter_invalid' => [ | ||
| '<?php class Test { public function test($invalid_param) {} }', | ||
| TRUE, | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Apply the same config wiring fix in camelCase parameter tests to resolve failures
testProcessWithCamelCaseFormat builds a separate Config and ruleset XML, but because the new config is not stored on $this->config, processCode() still uses the default DrevOps standard without the format="camelCase" property, so the sniff stays in snakeCase mode (matching the pipeline’s inverted expectations). Align the test with the base class by assigning the new config back before creating the ruleset:
try {
- $config->standards = [$ruleset_file];
- $this->ruleset = new Ruleset($config);
+ $config->standards = [$ruleset_file];
+ // Use this config for subsequent processCode() calls.
+ $this->config = $config;
+ $this->ruleset = new Ruleset($this->config);This should make the camelCase data provider cases pass (camelCase parameters valid, snake_case parameters flagged).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Test process method with camelCase format. | |
| * | |
| * @param string $code | |
| * PHP code to test. | |
| * @param bool $should_have_errors | |
| * Whether errors should be detected. | |
| */ | |
| #[DataProvider('dataProviderProcessWithCamelCaseFormat')] | |
| public function testProcessWithCamelCaseFormat(string $code, bool $should_have_errors): void { | |
| // Create a new config with camelCase format property. | |
| $config = new Config(); | |
| $config->standards = ['DrevOps']; | |
| $config->sniffs = ['DrevOps.NamingConventions.ParameterNaming']; | |
| // Create temporary ruleset XML with property configuration. | |
| $ruleset_xml = '<?xml version="1.0"?> | |
| <ruleset name="Test"> | |
| <rule ref="DrevOps.NamingConventions.ParameterNaming"> | |
| <properties> | |
| <property name="format" value="camelCase"/> | |
| </properties> | |
| </rule> | |
| </ruleset>'; | |
| $ruleset_file = tempnam(sys_get_temp_dir(), 'phpcs_ruleset_'); | |
| file_put_contents($ruleset_file, $ruleset_xml); | |
| try { | |
| $config->standards = [$ruleset_file]; | |
| $this->ruleset = new Ruleset($config); | |
| $file = $this->processCode($code); | |
| $errors = $file->getErrors(); | |
| if ($should_have_errors) { | |
| $this->assertNotEmpty($errors); | |
| } | |
| else { | |
| $this->assertEmpty($errors); | |
| } | |
| } | |
| finally { | |
| unlink($ruleset_file); | |
| } | |
| } | |
| /** | |
| * Data provider for camelCase format tests. | |
| * | |
| * @return array<string, array<mixed>> | |
| * Test cases. | |
| */ | |
| public static function dataProviderProcessWithCamelCaseFormat(): array { | |
| return [ | |
| 'valid_camel_case_parameter' => [ | |
| '<?php function test($validParameter) {}', | |
| FALSE, | |
| ], | |
| 'invalid_snake_case_parameter' => [ | |
| '<?php function test($invalid_parameter) {}', | |
| TRUE, | |
| ], | |
| 'single_word_valid' => [ | |
| '<?php function test($param) {}', | |
| FALSE, | |
| ], | |
| 'method_parameter_valid' => [ | |
| '<?php class Test { public function test($validParam) {} }', | |
| FALSE, | |
| ], | |
| 'method_parameter_invalid' => [ | |
| '<?php class Test { public function test($invalid_param) {} }', | |
| TRUE, | |
| ], | |
| ]; | |
| } | |
| /** | |
| * Test process method with camelCase format. | |
| * | |
| * @param string $code | |
| * PHP code to test. | |
| * @param bool $should_have_errors | |
| * Whether errors should be detected. | |
| */ | |
| #[DataProvider('dataProviderProcessWithCamelCaseFormat')] | |
| public function testProcessWithCamelCaseFormat(string $code, bool $should_have_errors): void { | |
| // Create a new config with camelCase format property. | |
| $config = new Config(); | |
| $config->standards = ['DrevOps']; | |
| $config->sniffs = ['DrevOps.NamingConventions.ParameterNaming']; | |
| // Create temporary ruleset XML with property configuration. | |
| $ruleset_xml = '<?xml version="1.0"?> | |
| <ruleset name="Test"> | |
| <rule ref="DrevOps.NamingConventions.ParameterNaming"> | |
| <properties> | |
| <property name="format" value="camelCase"/> | |
| </properties> | |
| </rule> | |
| </ruleset>'; | |
| $ruleset_file = tempnam(sys_get_temp_dir(), 'phpcs_ruleset_'); | |
| file_put_contents($ruleset_file, $ruleset_xml); | |
| try { | |
| $config->standards = [$ruleset_file]; | |
| // Use this config for subsequent processCode() calls. | |
| $this->config = $config; | |
| $this->ruleset = new Ruleset($this->config); | |
| $file = $this->processCode($code); | |
| $errors = $file->getErrors(); | |
| if ($should_have_errors) { | |
| $this->assertNotEmpty($errors); | |
| } | |
| else { | |
| $this->assertEmpty($errors); | |
| } | |
| } | |
| finally { | |
| unlink($ruleset_file); | |
| } | |
| } | |
| /** | |
| * Data provider for camelCase format tests. | |
| * | |
| * @return array<string, array<mixed>> | |
| * Test cases. | |
| */ | |
| public static function dataProviderProcessWithCamelCaseFormat(): array { | |
| return [ | |
| 'valid_camel_case_parameter' => [ | |
| '<?php function test($validParameter) {}', | |
| FALSE, | |
| ], | |
| 'invalid_snake_case_parameter' => [ | |
| '<?php function test($invalid_parameter) {}', | |
| TRUE, | |
| ], | |
| 'single_word_valid' => [ | |
| '<?php function test($param) {}', | |
| FALSE, | |
| ], | |
| 'method_parameter_valid' => [ | |
| '<?php class Test { public function test($validParam) {} }', | |
| FALSE, | |
| ], | |
| 'method_parameter_invalid' => [ | |
| '<?php class Test { public function test($invalid_param) {} }', | |
| TRUE, | |
| ], | |
| ]; | |
| } |
🧰 Tools
🪛 GitHub Actions: Test PHP
[error] 158-158: testProcessWithCamelCaseFormat@valid_camel_case_parameter: Failed asserting that an array is empty.
[error] 155-155: testProcessWithCamelCaseFormat@invalid_snake_case_parameter: Failed asserting that an array is not empty.
[error] 158-158: testProcessWithCamelCaseFormat@method_parameter_valid: Failed asserting that an array is empty.
[error] 155-155: testProcessWithCamelCaseFormat@method_parameter_invalid: Failed asserting that an array is not empty.
🪛 PHPMD (2.15.0)
128-164: The parameter $should_have_errors is not named in camelCase. (undefined)
(CamelCaseParameterName)
135-135: The variable $ruleset_xml is not named in camelCase. (undefined)
(CamelCaseVariableName)
144-144: The variable $ruleset_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
154-154: The variable $should_have_errors is not named in camelCase. (undefined)
(CamelCaseVariableName)
157-159: The method testProcessWithCamelCaseFormat uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
In tests/Unit/ParameterNamingSniffTest.php around lines 119-195, the test builds
a local $config and ruleset XML for camelCase but never assigns the new Config
back to the test instance, so processCode() still uses the original config
(keeping snake_case); fix this by wiring the new config into the test before
constructing the Ruleset (assign the local $config to $this->config), then
proceed to create the Ruleset and call processCode(); keep the temporary ruleset
file creation and unlink in the same try/finally block so teardown still runs.
The camelCase format tests used temporary XML rulesets to configure properties, which works with PHPCS 4 but fails with PHPCS 3. Since: - The camelCase functionality is already tested via reflection in AbstractVariableNamingSniffTest - The main snakeCase format functionality has full test coverage - CI tests against both PHPCS 3 and 4 Removed these tests to ensure compatibility across all PHPCS versions. Coverage remains at 99.51% with only 2 uncovered lines (error code ternary operators).
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.