Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Files/FileName: bug fix, support modern PHP and other improvements #342

Merged
merged 19 commits into from
Nov 19, 2023

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Nov 19, 2023

Files/FileName: minor documentation improvements

Files/FileName: efficiency fix

Always return a stack pointer at the end of the file, even when the file passed comes from STDIN, as we ignore STDIN files for this sniff, so there is no need to continue listening for tokens in the file.

Includes ignoring this line for code coverage checks as the line is untestable in a unit test situation.

Files/FileName: use PHPCSUtils in more places

Files/FileName: make non-interface methods private

As the sniff class is now final (since PR #319), there is no need for any protected methods, so let's make these private.

Files/FileName: minor code tweaks

Lower nesting levels and remove elseif when not needed.

👉 This commit will be easiest to review while ignoring whitespace changes.

Files/FileName: rename a local variable

$name is very generic, so let's rename this variable to $oo_name to make it more descriptive.

Files/FileName: more defensive coding for parse errors/live coding

Prevents a PHP notice in case of a IDE check during live coding.

Includes test.

👉 this change will be easiest to review while ignoring whitespace changes.

Files/FileName: improve error message consistency

Always have quotes around the expected + found file names.

As things were, this was not the case for the generic "lowercase/hyphenated" message + the class file name message.

Fixed now.

Files/FileName: examine views using only short echo open tags

Files using only short open tags were never examined. Fixed now.

Includes additional tests.

Files/FileName: allow for the inline PHPCS ignore annotations

These type of selective ignore annotations were introduced in PHPCS 3.2.0.

This commit now allows for disabling this sniff through an annotation, though error code based ignores are still not supported.

Includes unit tests.

Files/FileName: improve handling of files using non-underscore, non-dash word separators

This mirrors the same change as made upstream in the WPCS native FileName sniff in PR WordPress/WordPress-Coding-Standards#2285.

Files using non-underscore punctuation characters as word separators were previously not being flagged by the sniff.

Fixed now. Includes extra tests.

Files/FileName: refactor some near duplicate code

The switch cases for interfaces and traits are basically duplicates of each other with the term being variable.

This refactors the code to remove the duplicate code and use the term as a variable.

Includes updated unit tests to ensure the message creation is stable.

Files/FileName: add handling of files containing PHP 8.1+ enums

Enums should be treated the same as interfaces/traits, i.e.:

  • File name should reflect the name of the enum.
  • Prefixes should be stripped from the name.
  • The file name should end with -enum.

The sniff has been updated to handle enums in this way.

Includes additional unit tests.
Includes updated documentation.

Files/FileName: bug fix - excluded paths were not always normalized correctly

The is_file_excluded() method would first call the clean_custom_array_property() method with the $flip parameter set to true, resulting in an array containing the excluded files as the keys.

After that it would call the normalize_directory_separators() method on all values via an array_map().

In effect, this means that the actual values, which are stored in the array keys would still not be normalized, meaning the "is file excluded" check could return the wrong result for ruleset provided paths containing a backslash directory separator.

Even though, in practice, this didn't lead to problems as the paths in the Yoast rulesets are set using forward slashes, it is still a bug.

Fixed now by no longer "flipping" the array.

And as the $flip parameter of the clean_custom_array_property() method is now no longer used, the parameter and its handling has been removed from the function.

Includes test.

Files/FileName: simplification in excluded file comparison

There is no need to lowercase the "excluded files" + the file name for the is_file_excluded() check.

Checking these in their original, proper case should be just fine.

And as the $to_lower parameter of the clean_custom_array_property() method is now no longer used, the parameter and its handling has been removed from the function.

Includes additional test safeguarding that the comparison is done case-sensitively.

Files/FileName: only clean up the OO prefixes when needed

Efficiency tweak. Cleaning the ruleset passed OO prefixes doesn't need to be done time and again every single time this sniff is called.

For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient.

For test runs, this may need to be done more often, but that can be handled by storing the previous value of $oo_prefixes property and checking if it has changed before doing the validation.

This commit implements this.

Files/FileName: only clean up the excluded files when needed

Efficiency tweak. Cleaning the ruleset passed "excluded files" doesn't need to be done time and again every single time this sniff is called.

For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient.

For test runs, this may need to be done more often, but that can be handled by storing the previous value of $excluded_files_strict_check property and checking if it has changed before doing the validation.

This commit implements this.

Additionally it adds some extra safeguards for incorrectly passed "excluded files" paths.

Includes removing the $phpcsFile parameter from the is_file_excluded() method as it is no longer needed by that method.

Includes additional unit tests.
Includes updating a pre-existing test to pass duplicate excluded files in different ways.

Files/FileName: only throw the "missing basepath" warning once

No need to throw this for every single file. Throwing it once should be sufficient.

Includes moving the check for the missing basepath to earlier in the process flow.

Files/FileName: implement use of new PathHelper and PathValidationHelper classes

Always return a stack pointer at the end of the file, even when the file passed comes from `STDIN`, as we ignore `STDIN` files for this sniff, so there is no need to continue listening for tokens in the file.

Includes ignoring this line for code coverage checks as the line is untestable in a unit test situation.
As the sniff class is now `final` (since PR 319), there is no need for any `protected` methods, so let's make these `private`.
Lower nesting levels and remove `elseif` when not needed.

:point_right: This commit will be easiest to review while ignoring whitespace changes.
`$name` is very generic, so let's rename this variable to `$oo_name` to make it more descriptive.
Prevents a PHP notice in case of a IDE check during live coding.

Includes test.

:point_right: this change will be easiest to review while ignoring whitespace changes.
Always have quotes around the expected + found file names.

As things were, this was not the case for the generic "lowercase/hyphenated" message + the class file name message.

Fixed now.
Files using _only_ short open tags were never examined. Fixed now.

Includes additional tests.
These type of selective ignore annotations were introduced in PHPCS 3.2.0.

This commit now allows for disabling this sniff through an annotation, though error code based ignores are still not supported.

Includes unit tests.
…ash word separators

This mirrors the same change as made upstream in the WPCS native FileName sniff in PR WordPress/WordPress-Coding-Standards 2285.

Files using non-underscore punctuation characters as word separators were previously not being flagged by the sniff.

Fixed now. Includes extra tests.
The switch cases for `interface`s and `trait`s are basically duplicates of each other with the term being variable.

This refactors the code to remove the duplicate code and use the term as a variable.

Includes updated unit tests to ensure the message creation is stable.
Enums should be treated the same as interfaces/traits, i.e.:
* File name should reflect the name of the `enum`.
* Prefixes should be stripped from the name.
* The file name should end with `-enum`.

The sniff has been updated to handle enums in this way.

Includes additional unit tests.
Includes updated documentation.
…orrectly

The `is_file_excluded()` method would first call the `clean_custom_array_property()` method with the `$flip` parameter set to `true`, resulting in an array containing the excluded files as the keys.

After that it would call the `normalize_directory_separators()` method on all _values_ via an `array_map()`.

In effect, this means that the actual values, which are stored in the array _keys_ would still not be normalized, meaning the "is file excluded" check could return the wrong result for ruleset provided paths containing a backslash directory separator.

Even though, in practice, this didn't lead to problems as the paths in rulesets are set using forward slashes, it is still a bug.

Fixed now by no longer "flipping" the array.

And as the `$flip` parameter of the `clean_custom_array_property()` method is now no longer used, the parameter and its handling has been removed from the function.

Includes test.
There is no need to lowercase the "excluded files" + the file name for the `is_file_excluded()` check.

Checking these in their original, proper case should be just fine.

And as the `$to_lower` parameter of the `clean_custom_array_property()` method is now no longer used, the parameter and its handling has been removed from the function.

Includes additional test safeguarding that the comparison is done case-sensitively.
Efficiency tweak. Cleaning the ruleset passed OO prefixes doesn't need to be done time and again every single time this sniff is called.

For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient.

For test runs, this may need to be done more often, but that can be handled by storing the previous value of `$oo_prefixes` property and checking if it has changed before doing the validation.

This commit implements this.
Efficiency tweak. Cleaning the ruleset passed "excluded files" doesn't need to be done time and again every single time this sniff is called.

For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient.

For test runs, this may need to be done more often, but that can be handled by storing the previous value of `$excluded_files_strict_check` property and checking if it has changed before doing the validation.

This commit implements this.

Additionally it adds some extra safeguards for incorrectly passed "excluded files" paths.

Includes removing the `$phpcsFile` parameter from the `is_file_excluded()` method as it is no longer needed by that method.

Includes additional unit tests.
Includes updating a pre-existing test to pass duplicate excluded files in different ways.
No need to throw this for every single file. Throwing it once should be sufficient.

Includes moving the check for the missing basepath to earlier in the process flow.
@coveralls
Copy link

Coverage Status

coverage: 98.617% (+0.04%) from 98.58%
when pulling b94cf92 on JRF/yoastcs-filename-various-improvements
into f17dfbe on develop.

@jrfnl jrfnl merged commit e6c1b98 into develop Nov 19, 2023
26 checks passed
@jrfnl jrfnl deleted the JRF/yoastcs-filename-various-improvements branch November 19, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants