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

New Universal.DeclareStatements.DeclareStatementsStyle sniff #129

Open
wants to merge 126 commits into
base: develop
Choose a base branch
from

Conversation

dingo-d
Copy link
Contributor

@dingo-d dingo-d commented Sep 23, 2022

This sniff will check the declare statements for a specific coding style.

By default, the style of the declare statement is without braces and will be applied for all the directives (strict_types, ticks, encoding).

The sniff can be configured to require or disallow braces (except for the strict_types), and selectively select for which directive the selected style should be enforced (ticks, encoding).

  • Includes tests
  • Includes documentation
  • Includes metrics

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Reviewed during pair programming session in which the review comments were discussed (which is why they are sometimes a bit shorthand).

Additional remarks/things to think over:

  • If there is a declare statement with multiple directives and those directive have different requirements, i.e, strict_types: disallow and ticks set to require: how should this be handled by the sniff ?

For potentially adding fixers - helpful link: https://pear.php.net/package/PHP_CodeSniffer/docs/3.5.4/apidoc/PHP_CodeSniffer/Fixer.html

General remark: in the sniffs, everything which gets a docblock ought to get a @since tag in this repo (can't be enforced yet as there's no good sniff for it, but will be enforced in the future).
For the tests, just the @since at class level is fine.

@jrfnl
Copy link
Member

jrfnl commented Sep 23, 2022

Some ideas we discussed about adding metrics:

metric name "Declare statement modus"

values:

  • block mode
  • single-line

alternative style values:

  • implode(', ', $directiveStrings) . block mode
  • implode(', ', $directiveStrings) . single line mode

@dingo-d
Copy link
Contributor Author

dingo-d commented Sep 25, 2022

I've reworked the sniff and tests, but still need to work on the fixer and metrics.

Also, I expect there will be some remarks on the PR, those are definitely welcomed 🙂

@jrfnl jrfnl removed this from the 1.0.0-alpha4 milestone Oct 25, 2022
@jrfnl
Copy link
Member

jrfnl commented Oct 25, 2022

FYI: I'm moving this PR out of the 1.0.0-alpha4 milestone as I'm hoping to release over the next few days.

@dingo-d dingo-d force-pushed the declare-statements-curly-brackets branch from 516e6db to a775e65 Compare December 3, 2022 09:45
@dingo-d
Copy link
Contributor Author

dingo-d commented Dec 3, 2022

@jrfnl I've added some metrics, but I'm unsure if this is correct.

When I run the report for metrics, I get correct percentages per file (all adds up to 100%) but I'm not sure if I'm doing it right, as in one file (3rd test file) I have multiple block directives. Some are ok, some are not, and the 2 are ok, but for some reason they are being counted as separate directives, and the type isn't correctly recognized:

vendor/bin/phpcs --standard=universal --sniffs=universal.declarestatements.blockmode Universal/Tests/DeclareStatements/BlockModeUnitTest.3.inc -sp --report=info

E 1 / 1 (100%)

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Declare directive scope: Block [10/10, 100%]

Declare directive type: encoding [4/4, 100%]

----------------------------------------------------------------------
Time: 12.06 secs; Memory: 6MB

When debugging, the value is different (first encoding then ticks) but only encoding is reported.

I wanted to group the types, and they seem to be correctly recognized when they are on their own.

I still need to check if there are any fixable errors. The only fixable case I can think of could be if there is only one directive in the declare statement that is using block mode when it shouldn't. And then I need to be careful to correctly replace the closer, add semicolon, etc.

Edit:

I tried to add the fixer, but I got stuck, as there are tons of whitespace edge cases that can trip me up. I did save my WIP for the fixer in another branch so I may play with it later, but for now I'm not sure it's worth the time spent 🤷🏼‍♂️

@dingo-d dingo-d marked this pull request as ready for review December 3, 2022 09:51
@jrfnl jrfnl force-pushed the declare-statements-curly-brackets branch from aac4f3a to 5834c64 Compare December 9, 2022 17:13
jrfnl and others added 8 commits May 28, 2023 10:13
New sniff to detect using `static` instead of `self` in OO constructs which are `final`.

Includes fixer.
Includes unit tests.
Includes documentation.
New sniff to enforce no spaces around union type and intersection type separator operators.

Includes fixer.
Includes unit tests.
Includes documentation.
Includes metrics.

Co-authored-by: Denis Žoljom <[email protected]>
… array opener

... when a new line is required after the opener of a multi-line array.

This sniff should not have an opinion on whether or not trailing comments are allowed. There are other sniffs around to forbid those, if so desired.

Includes extra unit tests to safeguard this change and to safeguard handling of comments after the array opener regardless of this change.
New sniff to enforce indentation to always be a multiple of a tabstop, i.e. disallow precision alignment.

For space-based standards, the `Generic.WhiteSpace.DisallowTabIndent` sniff (unintentionally/incorrectly) already removes precision alignment.
For tab-based standards, the `Generic.WhiteSpace.DisallowSpaceIndent` sniff, however, does not remove precision alignment.
With that in mind, this sniff is mostly intended for standards which demand tabs for indentation.

In rare cases, spaces for precision alignment can be intentional and acceptable, but more often than not, precision alignment is a typo.

Notes:
* This sniff does not concern itself with tabs versus spaces.
    Use the PHPCS native `Generic.WhiteSpace.DisallowTabIndent` or the `Generic.WhiteSpace.DisallowSpaceIndent` sniffs for that.
* When using this sniff with tab-based standards, please ensure that the `tab-width` is set and either don't set the `$indent` property or set it to the tab-width (or a multiple thereof).
* Precision alignment *within* text strings (multi-line text strings, heredocs, nowdocs) will NOT be flagged by this sniff.
* Precision alignment in (trailing) whitespace on otherwise blank lines will not be flagged by default.
    Most standards will automatically remove trailing whitespace anyway using the `Squiz.WhiteSpace.SuperfluousWhitespace` sniff.
    If the Squiz sniff is not used, set the `ignoreBlankLines` property to `false` to enable reporting for this.
* The sniff also supports an option to ignore precision alignment in specific situations, like for multi-line chained method calls.

**Note**: the fixer works based on "best guess" and may not always result in the desired indentation. Combine this sniff with the `Generic.WhiteSpace.ScopeIndent` sniff for more precise indentation fixes.
If so desired, the fixer can be turned off by including the rule in a standard like so: `<rule phpcs-only="true" ref="Universal.WhiteSpace.PrecisionAlignment"/>`

As it is, the sniff supports both PHP, as well as JS and CSS, though the support for JS and CSS should be considered incidental and will be removed once PHPCS 4.0 will be released.

Includes fixer.
Includes unit tests.
Includes documentation.
Checks the amount of spacing between the `class` keyword and the open parenthesis (if any) for anonymous class declarations.

The expected amount of spacing is configurable and defaults to `0`.

The default value of `0` has been decided upon based on scanning a not insignificant number of codebases and determining the prevailing style used for anonymous classes.
It is also in line with the previously proposed [errata for PSR12](php-fig/fig-standards#1206).

Includes fixer.
Includes unit tests.
Includes documentation.
Includes metrics.
This file is not supposed to yield errors/warnings, so the `.fixed` file is redundant.
jrfnl and others added 30 commits May 28, 2023 10:13
... now upstream PR 3639 has been merged.

Ref:
* squizlabs/PHP_CodeSniffer 3639
…tion

... as long as the arrow function is within an OO construct which can be `final`.

Includes tests.
Includes enhancing the closure test a little as well.
PHPCSDevCS was previously not added to the `require-dev` dependencies as the minimum supported PHPCS version conflicted with the minimum supported PHPCS version of this package.

Now this is no longer the case, the package can be safely added to `require-dev` and work-arounds can be removed.
These steps/directives all relate to testing against PHPCS 4.x, but this package is not being tested against PHPCS 4.x at this time and when this will be enabled again, these work-arounds may well no longer be needed anymore anyway, so let's remove it for now and re-evaluate what is needed when testing against PHPCS 4.x is re-enabled.
* Update the PHP version on which the CS run for this package is run to PHP `latest`.
* Remove the `continue-on-error` for PHP 8.2 in the `test` workflow (and remove the `experimental` key, which is now no longer used.
* Update the "Tested against" badge in the README.
* Update links to workflows.
* Use re-usable links for links which are used repeatedly.
... to be before the adjustments made to the composer.json file.
_Based on a similar workflow previously added to PHPCSUtils._

Remark offers a number of "rules" which are useful, such as checking that links and link definitions match up, verifying all used links work and some formatting checks.

The rule configuration is contained in the .remarkrc file.

Files/directories to be ignored can be listed in the .remarkignore file which supports glob syntax, like .gitignore.
Note: .-prefixed files and directories are excluded by default. To include those in the scan, they have to be passed explicitly on the command-line.

To run locally, follow the same steps as per the GitHub actions workflow.

Note: the workflow contains a double-run of remark-lint to allow viewing the results both in a human readable way in the actions transscripts, as well as getting annotations for found issue in PRs.

Refs:
* https://github.com/remarkjs/remark/tree/main/packages/remark-cli
* https://github.com/remarkjs/remark-lint
* https://github.com/remarkjs/remark-gfm
* https://github.com/remarkjs/remark-lint/tree/main/packages/remark-preset-lint-consistent
* https://github.com/remarkjs/remark-lint/tree/main/packages/remark-preset-lint-recommended
* https://github.com/remarkjs/remark-lint/tree/main/packages/remark-preset-lint-markdown-style-guide

Additional (external) plugins included:
* https://github.com/vhf/remark-lint-heading-whitespace
* https://github.com/wemake-services/remark-lint-list-item-punctuation
* https://github.com/laysent/remark-lint-plugins/tree/HEAD/packages/remark-lint-match-punctuation
* https://github.com/olizilla/remark-lint-no-hr-after-heading
* https://github.com/wemake-services/remark-lint-are-links-valid
* https://github.com/remarkjs/remark-validate-links
... to loosely safeguard valid and consistent yaml files.

Includes adding a configuration file which loosens up the default ruleset to a degree I'm comfortable with.

Ref: https://yamllint.readthedocs.io/
Includes a few updated unit tests to safeguard the fix.
... of the bash `date` command in the earlier pulled cache busting.
PHP 8.2 has been released today 🎉 and the `setup-php` action has announced support for PHP 8.3, so adding PHP 8.3 to the matrix and no longer allowing PHP 8.2 to fail the build.

Note: PHPCS does not (yet) have full syntax support for PHP 8.2, but it does have runtime support (for the most part, see squizlabs/PHP_CodeSniffer 3629).

Builds against PHP 8.3 are still allowed to fail for now.
Follow up on 186 which added some new configuration files, but didn't export ignore them.

Includes aligning the values for better readability.
Will probably need some fixup.
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.

3 participants