Generic/IncrementDecrementSpacing: add XML documentation

[rodrigoprimo]

Description

This PR adds the XML documentation for the Generic.WhiteSpace.IncrementDecrementSpacing sniff.

Suggested changelog entry

Add XML documentation for the Generic.WhiteSpace.IncrementDecrementSpacing sniff.

Related issues/external references

Part of #148

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

Hi @rodrigoprimo, thanks for this PR.

I wonder if it would make sense to simplify these docs to just one standard block and one set of code samples dealing with both pre- as well as post-in/decrement ?
Was there a particular reason to split this into two standards ? After all, the rule is the same "no space between the increment/decrement operator and the "variable" it applies to", doesn't really matter whether it's pre or post in/decrement.

I also wonder if the highlighting via the <em> tags could be made more meaningful. Would it make sense to include the operator in the tags ? and the operator + whitespace for the invalid cases ?

[rodrigoprimo]

I wonder if it would make sense to simplify these docs to just one standard block and one set of code samples dealing with both pre- as well as post-in/decrement ?
Was there a particular reason to split this into two standards ?

I was more inclined to use a single block, but I was a bit unsure, so I opted to follow what is recommended in the documentation and created one block per error/warning. I pushed a new commit combining the two blocks into one.

I also wonder if the highlighting via the <em> tags could be made more meaningful. Would it make sense to include the operator in the tags ? and the operator + whitespace for the invalid cases ?

In my opinion, it is better to leave just the whitespaces or the "point" between the variable and the operator in the <em> tags. In the HTML version, this "point" is highlighted, and to me, it is clear that it is marking the place where the sniff is checking that there is no whitespace, and thus, the code is valid (maybe the only exception that I don't think is ideal is when there is a newline as only the place in the first line is highlighted and nothing is highlighted in the second line):

Could you please elaborate on why highlighting the operator would make the <em> tags more meaningful? I am happy to implement that if you still prefer it.

On a side note, the guide on how to write sniff documentation mentions that the HTML and Markdown generators use the <em> tags, but I don't see anything highlighted when I use the Markdown generator. Is this a bug, or should the documentation be updated? Or maybe I'm missing something and the <em> tags are included?

I tested with the following command:

bin/phpcs --standard=Generic --generator=Markdown --sniffs=Generic.WhiteSpace.IncrementDecrementSpacing

[jrfnl]

Thanks for the update. A few more grammar fixes to go and this is ready for merge.

In the HTML version, this "point" is highlighted, and to me, it is clear that it is marking the place where the sniff is checking

Fair point. Let's leave the <em> tags as they are now.

I don't see anything highlighted when I use the Markdown generator. Is this a bug, or should the documentation be updated?

Might well be a bug. I haven't really looked at that class before.
Then again, if generating in pure markdown - how would you highlight something within a code sample ?
The markdown specs don't really allow for that, other than via a mix of Markdown and HTML, so I suspect the documentation should be updated, not the class.

[jrfnl]

"whitespaces" is not a word.

I think either one of the following could work:

• "One or more whitespace characters between..."
• "Whitespace between..."

[rodrigoprimo]

Ops, I didn't know that. Thanks!

I pushed a new commit, replacing the three instances where I used "whitespaces".

[jrfnl]

Suggested change

	<code title="Valid: No whitespaces between variables and increment/decrement operators.">
	<code title="Valid: No whitespace between variables and increment/decrement operators.">

[jrfnl]

Suggested change

	There should be no whitespaces between variables and increment/decrement operators.
	There should be no whitespace between variables and increment/decrement operators.

[rodrigoprimo]

Might well be a bug. I haven't really looked at that class before.
Then again, if generating in pure markdown - how would you highlight something within a code sample ?
The markdown specs don't really allow for that, other than via a mix of Markdown and HTML, so I suspect the documentation should be updated, not the class.

I haven't checked the class that generates the Markdown documentation. I don't know if it changes anything, but I noticed that it is already mixing Markdown and HTML. Also, I don't know if and where the Markdown documentation is used.

I'm also inclined to think that we should just update the documentation for now. Happy to do it if you would like.

[jrfnl]

I'm also inclined to think that we should just update the documentation for now. Happy to do it if you would like.

I've included that change in PR #304

[jrfnl]

I haven't checked the class that generates the Markdown documentation. I don't know if it changes anything, but I noticed that it is already mixing Markdown and HTML. Also, I don't know if and where the Markdown documentation is used.

The Markdown generator was added in squizlabs/PHP_CodeSniffer#381.

The generator output isn't used by PHPCS itself at this time, but, aside from direct help via CLI, the HTML and Markdown output can be added to a website or wiki for a (project) coding standard. To me, that's the use-case for those generators.

HTML can be used in Markdown, it is valid, but if the output becomes largely HTML, then I don't see why there should be a second (separate) generator for Markdown, so for now, I'm inclined to leave it alone.

At a future point in time, I'd like to have a think about adding info related to public properties which can be set on a sniff to the docs and adding the possibility of adding links (like to the official PSR12 docs for the PSR12 sniffs) to docs.
That would be the time to have a closer look at the generator classes for me.

First priority regarding the docs now, is getting the docs for each sniff in place and there is still enough work to do there.
After that we can look at enhancing the available information.

Does that make sense ?

[jrfnl]

Thanks @rodrigoprimo. LGTM.

I did notice a tab had creeped into one of the code samples, I'll fix that up when I rebase the PR before merging it.