squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Generic.Formatting.SpaceAfterNot settings not reflected in generated documentation. #3835

Closed NathanGibbs3 closed 1 year ago

NathanGibbs3 commented 1 year ago

Describe the bug Generic.Formatting.SpaceAfterNot settings not reflected in generated documentation.

Command line used: phpcs -q --generator=Markdown > "./docs/BASE-Coding-Standards.md" Input configuration file: https://github.com/NathanGibbs3/BASE/blob/master/phpcs.xml.dist Output documentation file generated by phpcs: https://github.com/NathanGibbs3/BASE/blob/master/docs/BASE-Coding-Standards.md

The configuration specifies <property name="spacing" value="0" /> The phpcs generated documentation lists the following as an example of invalid code. if (!$someVar || !$x instanceOf stdClass) {};

This does not match the configured setting.

Code sample N/A

Custom ruleset N/A

To reproduce Steps to reproduce the behavior:

  1. Pull down to Repo referenced above. https://github.com/NathanGibbs3/BASE
  2. Run phpcs -q --generator=Markdown > "./docs/BASE-Coding-Standards.md"
  3. See the generated BASE-Coding-Standards.md in the docs directory.

PHPCS output here None on console, file generated in ./docs/BASE-Coding-Standards.md

Expected behavior Expected the phpcs generated documentation to list if (!$someVar || !$x instanceOf stdClass) {}; as an example of valid code.

Versions (please complete the following information):

Additional context

jrfnl commented 1 year ago

@NathanGibbs3 I hear you, but that's not how the documentation works. The documentation shows the default functionality of the sniff and does not take the ruleset into account. It's literally a hard-coded XML file.

A PR to improve the documentation and possibly add a mention that the behaviour of the sniff can be changed via configurable properties could be considered, but the base of the documentation should reflect the default property values.

NathanGibbs3 commented 1 year ago

OK, my misunderstanding. I thought the documentation created by --generator= followed the configuration file and could be used by developers to quickly update per project coding standards documents that future contributors to the project could reference. Basically, if the project coding standard changed, even in a small way, one run of phpcs could automatically update the relevant contributing code guidelines.

It would be a nice feature to have, but I understand, that as it is right now, this is not a bug.

jrfnl commented 1 year ago

@NathanGibbs3 Well, the docs shown via the --generator feature include only those sniffs included in the ruleset, so if new sniffs get added to the ruleset or sniffs get removed, re-running the docs will get you an up-to-date set of docs, but the contents of the documentation for each sniff is hard-coded (as things are at this time).

NathanGibbs3 commented 1 year ago

Implementing what I thought, would be a big project. Making things change around in the generated documentation based on user configuration options for each sniff, could get interesting, as some sniffs have several options.

jrfnl commented 1 year ago

@NathanGibbs3 Exactly. I do think it's an interesting idea, but I also think there is not enough bandwidth to create and maintain that feature for this project as things are at this time.

NathanGibbs3 commented 1 year ago

I figured out why I misunderstood this feature. The title of the generated documents are set to $ProjectName Coding Standards, so naturally, I thought the generated document was documenting the configured sniff options for the project. It may be worth considering changing the title of the generated documents to reflect what they actually are. Maybe something like: $ProjectName Included Sniffs PHP_CodeSniffer Default Sniff Configuration

NathanGibbs3 commented 1 year ago

@NathanGibbs3 Well, the docs shown via the --generator feature include only those sniffs included in the ruleset, so if new sniffs get added to the ruleset or sniffs get removed, re-running the docs will get you an up-to-date set of docs,

That alone, gets us most of the way to automatically generating project coding standards documentation.

but the contents of the documentation for each sniff is hard-coded (as things are at this time).

I took a quick look at the code. It appears that the generator, processes the hard coded XML file, with the assumption that the first provided code example is the valid one, and the second is the invalid one.

Some preprocessing code to:

That might get us the rest of the way there. The generator code & hard coded XML files may not even have to be changed.

NathanGibbs3 commented 1 year ago

with the assumption that the first provided code example is the valid one, and the second is the invalid one.

If the above assumption is true for all sniffs produced for this tool, then a simplified preprocessor would look like

if ( Sniff has options set in config ){
    // It is not longer in it's default state.
    Invert order of code examples in XML stream.
}

Personally, I don't recommend that solution, as it builds assumptions on top of assumptions.

jrfnl commented 1 year ago

@NathanGibbs3 Unfortunately, it's not as simple as that.

Yes, the order of valid/invalid examples is quite standardized, but no, options don't necessarily mean that invalid becomes valid and visa versa.

There is a wide range of options and how they work depends on each individual sniff. You can find the list of available properties in PHPCS itself here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties

NathanGibbs3 commented 1 year ago

@NathanGibbs3 Unfortunately, it's not as simple as that.

Yes, the order of valid/invalid examples is quite standardized, but no, options don't necessarily mean that invalid becomes valid and visa versa.

Precisely why the "simplified" preprocessor idea isn't the recommended solution. :smile: It would just add more technical debt to the situation.

Probably, the first preprocessor idea would be the preferred solution.

Concerning report generation. If it's not too difficult to alter the XML stream going into the generator, or to control where the generator is getting it's XML from, a tmp file or in memory data structure created by the preprocessor maybe, then that would get us closer. I have not had time to figure out where in the PHPCS codebase that functionality is located.

Basically if my future self, or anyone else wants to pick this up and make it work, they have some initial thinking around it captured in this thread.

There is a wide range of options and how they work depends on each individual sniff. You can find the list of available properties in PHPCS itself here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties

Yes, my favorite piece of PHPCS documentation. :smile:

jrfnl commented 1 year ago

We could re-open this issue with an adjusted title as a feature request, but to be honest, in my opinion, it will be very low prio considering everything else which is open, and the fact that the documentation as-is is far from complete in the first place.

Maybe at a later time when we are in a better place with regards to the backlog.

NathanGibbs3 commented 1 year ago

We could re-open this issue with an adjusted title as a feature request, but to be honest, in my opinion, it will be very low prio considering everything else which is open, and the fact that the documentation as-is is far from complete in the first place.

I will leave that to your discretion. I understand the frustration of less than complete documentation. I also know what it feels like to be buried in bug reports & feature requests. :computer: :smile: Low priority is fine with me. 👍

Maybe at a later time when we are in a better place with regards to the backlog.

I understand completely. As a project maintainer, if you feel that this idea is worth consideration at a future time, re-open it. :smile: