Minor fixes and adjustments to the PHPCS 4.0 upgrade guides #44
Conversation
|
=== This is an auto-generated comment === Thank you for your PR. Please review the resulting final markdown files via the created artifact. N.B.: the above link will automatically be updated when this PR is updated. |
|
I believe the |
rodrigoprimo
added
Type: other documentation improvement
and removed
Status: triage
labels
Indeed. Might be related to a change in Lychee. If the build on the main branch also fails once this is merged, I guess I'll have to look into this. |
jrfnl
changed the title
There was a problem hiding this comment.
Thanks for this PR @rodrigoprimo ! Left feedback on a few things, most of it looks okay though.
| Typically, these type of workarounds can be found by searching for calls to the `Ruleset::registerSniffs()` method. | ||
| Typically, these types of workarounds can be found by searching for calls to the `Ruleset::registerSniffs()` method. |
There was a problem hiding this comment.
Not sure about this one. I believe singular is correct here.
There was a problem hiding this comment.
I'm not 100% sure as well, and thus I'm ok to leave it as is. Grammarly marks it as an agreement mistake, but it could be a false positive.
An alternative might be "Typically, these workarounds can be found by searching for calls to the Ruleset::registerSniffs() method." or even "Typically, these can be found by searching for calls to the Ruleset::registerSniffs() method."
There was a problem hiding this comment.
I suggest leaving it as is (as in: the original text).
There was a problem hiding this comment.
Ok, I reverted my change.
| @@ -352,7 +352,7 @@ Previously a ruleset could already "extend" an array property for a sniff set by | |||
|
|
|||
| As of PHP_CodeSniffer 4.0, a ruleset can also "extend" the default value of an array property as set in the sniff itself. | |||
|
|
|||
| The upside of this is, that if you want to default value + some extras, you no longer need to duplicate the default values from sniff array properties in your ruleset. | |||
| The upside of this is that if you want the default value + some extras, you no longer need to duplicate the default value from a sniff array property in your ruleset. | |||
There was a problem hiding this comment.
I believe I intended to write "want to use the default value + ..."
As for the change in the second part of the sentence... I still think plural is correct here.
There was a problem hiding this comment.
I believe I intended to write "want to use the default value + ..."
So something like the sentence below?
"The upside of this is that, if you want to use the default value + some extras, you no longer need to duplicate the default values from sniff array properties in your ruleset."
As for the change in the second part of the sentence... I still think plural correct here.
I'm not sure about this one as well. In the version that I suggested above, plural seems correct to me as well.
There was a problem hiding this comment.
The proposed sentence looks good to me.
There was a problem hiding this comment.
Ok, I updated the sentence with the proposal we discussed.
Confirmed as an upstream issue introduced in Lychee 0.19.0. |
|
@rodrigoprimo I've now pulled & merged a fix which will prevent the build failure. Could you please rebase the PR when you address the review feedback ? That should get you a passing build. |
rodrigoprimo
force-pushed
the
phpcs-4-upgrade-guides
branch
from
d830cd4 to
b8931a5
Compare
Rebased without changes to get the build passing. I responded to your comments, but still haven't made any changes as there are some details that I would like to discuss with you. |
rodrigoprimo
force-pushed
the
phpcs-4-upgrade-guides
branch
from
d6b5d7f to
a20fb32
Compare
rodrigoprimo
force-pushed
the
phpcs-4-upgrade-guides
branch
from
a20fb32 to
28118ed
Compare
Description
This PR makes some minor fixes and adjustments to the two PHPCS 4.0 upgrade guides.
Related issues/external references
Fixes #2