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

Create special case for T_BAD_CHARACTER constant #1586

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Jul 25, 2023

Fixes #1351

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.

Hiya @fredden Thanks for looking into this!

I need to think this solution over a little as it is quite different from what I had in mind.

Would you mind explaining why you choose to create a separate array based sniff, instead of special casing the constant in the RemovedConstants and NewConstants sniffs ?

Do you expect this kind of thing to happen more regularly in PHP in the future ?

@jrfnl jrfnl added this to the 10.0.0 milestone Jul 25, 2023
@fredden
Copy link
Contributor Author

fredden commented Jul 25, 2023

Would you mind explaining why you choose to create a separate array based sniff, instead of special casing the constant in the RemovedConstants and NewConstants sniffs ?

Initially I added more code to RemovedConstants to create a special case for this constant. When I came to do the same for NewConstants, I realised that I was writing a lot of (what looked like) duplicate code. Also, fitting these special cases into the existing unit tests seemed difficult.

If you'd prefer, I can look at re-doing this without adding a new sniff and instead only modifying RemovedConstants and NewConstants sniffs (maybe with a new trait to avoid duplication).

Do you expect this kind of thing to happen more regularly in PHP in the future ?

I have no idea. :) I suspect it won't re-occur, but I don't know.

@jrfnl
Copy link
Member

jrfnl commented Jul 25, 2023

Would you mind explaining why you choose to create a separate array based sniff, instead of special casing the constant in the RemovedConstants and NewConstants sniffs ?

Initially I added more code to RemovedConstants to create a special case for this constant. When I came to do the same for NewConstants, I realised that I was writing a lot of (what looked like) duplicate code. Also, fitting these special cases into the existing unit tests seemed difficult.

Just guessing - were you trying to make the special casing dynamic ? As in: allow for an array based list of special cases ? In that case, I can imagine there would be redundancy and code duplication, but as things are, we only need to account for one special case....

Do you expect this kind of thing to happen more regularly in PHP in the future ?

I have no idea. :) I suspect it won't re-occur, but I don't know.

Well, as neither of us can look into the future... what about writing the sniff based on the now ?

What I mean by that, is that the sniff as-is doesn't need to be array based.
It is only dealing with one specific constant and we know all we need to know about that constant, so unless (and until) a second constant would be removed and then re-added to PHP, there is no need for the sniff to have all the extra flexibility (and complexity) of being an array based sniff.

This would also allow for greatly simplifying the tests.

If you'd prefer, I can look at re-doing this without adding a new sniff and instead only modifying RemovedConstants and NewConstants sniffs (maybe with a new trait to avoid duplication).

Let me think it over a little more ;-)

In the mean time - would you like some general feedback on the code as-is ? (without an opinion on the direction of the PR yet)

@fredden
Copy link
Contributor Author

fredden commented Jul 25, 2023

were you trying to make the special casing dynamic ? As in: allow for an array based list of special cases ?

Yes, I had avoided hard-coding the T_BAD_CHARACTER name and listed this in a (array) class constant/property.

Well, as neither of us can look into the future... what about writing the sniff based on the now ?

What I mean by that, is that the sniff as-is doesn't need to be array based. It is only dealing with one specific constant and we know all we need to know about that constant, so unless (and until) a second constant would be removed and then re-added to PHP, there is no need for the sniff to have all the extra flexibility (and complexity) of being an array based sniff.

Looking at the code, there's not a whole lot in there that would be simpler if we hard-coded everything rather than looking up values from the array property. Perhaps I've misunderstood what you mean.

This would also allow for greatly simplifying the tests.

I can see that this would allow removing a few arguments in the data provider. I wanted to use yield in the data provider, which would have made it a lot cleaner, but the project still supports PHP 5.4 and yield was introduced in PHP 5.5.

It's possible to reduce the number of tests without impacting coverage - for example, we don't strictly need to test with all of 5.0 and 5.1 and 5.2 etc; picking only some versions would be enough to show the code works as expected.
Would you like me to reduce the tests?

In the mean time - would you like some general feedback on the code as-is ? (without an opinion on the direction of the PR yet)

Yes, that sounds useful for progressing this work.

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.

As noted in my previous comment - these are mostly just general feedback/guidelines for how we work in this repo without an opinion on the direction this PR should take.

If we'd go with the new sniff, I would like to have a good think about the sniff name as well.
While it is a single-constant sniff it could just be named after the constant.
And maybe use Reintroduced instead of TemporarilyRemoved ?

@jrfnl
Copy link
Member

jrfnl commented Feb 6, 2024

@fredden and me discussed this off-GitHub and what with this being a breaking change as it currently is (new sniffname/error codes), while it could also be special cased in the NewConstants and RemovedConstants, I've stated a preference for doing the special casing instead and @fredden has indicated he's willing to work on a new iteration for this.

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.

@fredden Thanks for updating the PR. Looks like this could still be simplified further.

Would you like me to push an update to the PR ?

@fredden
Copy link
Contributor Author

fredden commented Feb 12, 2024

@jrfnl thanks for the feedback. I've applied the suggested changes and confirmed that the test suite still runs without error. Would you like to squash-merge this, or shall I rewrite the history to have only one commit here (after you've reviewed again)?

@fredden fredden requested a review from jrfnl February 12, 2024 15:36
@jrfnl
Copy link
Member

jrfnl commented Feb 12, 2024

@jrfnl thanks for the feedback. I've applied the suggested changes and confirmed that the test suite still runs without error. Would you like to squash-merge this, or shall I rewrite the history to have only one commit here (after you've reviewed again)?

@fredden Thanks for making that update and for your work on this PR! I'll squash-merge it once the build passes.
I did push a couple of tiny tweaks still to address earlier review remarks (#1586 (comment)), but nothing significant.

Feedback-wise, the only thing which I'd still say about this PR (but which is definitely not a blocker) is that splitting the tests and removing some redundancy would still be nice (see #1586 (comment) ), but I'm fine with leaving that.

@jrfnl jrfnl merged commit e5cd2e2 into PHPCompatibility:develop Feb 12, 2024
40 checks passed
@fredden fredden deleted the t-bad-character branch March 15, 2024 18:04
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.

The constant "T_BAD_CHARACTER" is removed since PHP 7.0
2 participants