Skip to content

Keywords/NewKeywords: merge NewArrowFunctionSniff#1593

Merged
wimg merged 1 commit intodevelopfrom
feature/newkeywords-merge-arrow-fn-sniff
Aug 4, 2023
Merged

Keywords/NewKeywords: merge NewArrowFunctionSniff#1593
wimg merged 1 commit intodevelopfrom
feature/newkeywords-merge-arrow-fn-sniff

Conversation

@jrfnl
Copy link
Copy Markdown
Member

@jrfnl jrfnl commented Aug 4, 2023 •
edited
Loading

The NewArrowFunction sniff was created prior to the decision to raise the minimum supported PHPCS version to 3.7.1.

As for older PHPCS versions, a lot of custom logic was needed to recognize arrow functions, it made sense to have a separate sniff for this.

However, when support for PHPCS < 3.7.1 was dropped, the sniff was slimmed down to not contain any logic anymore, so we may as well merge the detection of arrow functions into the NewKeywords sniff.

This is not a BC break as the NewArrowFunctionSniff was new to PHPCompatibility 10.0.0 anyway.

Note: all tests from the original NewArrowFunction sniff have been moved over and merged into the NewKeywords test file.

Related to #808

@jrfnl
Keywords/NewKeywords: merge NewArrowFunctionSniff
f6dc7fc 
The `NewArrowFunction` sniff was created prior to the decision to raise the minimum supported PHPCS version to 3.7.1.

As for older PHPCS version, a lot of custom logic was needed to recognize arrow functions, it made sense to have a separate sniff for this.

However, when support for PHPCS < 3.7.1 was dropped, the sniff was slimmed down to not contain any logic anymore, so we may as well merge the detection of arrow functions into the `NewKeywords` sniff.

This is **not** a BC break as the `NewArrowFunctionSniff` was new to PHPCompatibility 10.0.0 anyway.

Note: all tests from the original `NewArrowFunction` sniff have been moved over and merged into the `NewKeywords` test file.

Related to 808
@jrfnl jrfnl added Type: chores/QA PR: quick merge PR only contains relatively simple changes PR: ready for review labels Aug 4, 2023
@jrfnl jrfnl added this to the 10.0.0 milestone Aug 4, 2023
@jrfnl jrfnl requested a review from wimg August 4, 2023 14:58
@wimg wimg merged commit a963596 into develop Aug 4, 2023
@wimg wimg deleted the feature/newkeywords-merge-arrow-fn-sniff branch August 4, 2023 18:15
@github-actions github-actions bot removed PR: quick merge PR only contains relatively simple changes PR: ready for review labels Aug 4, 2023
@jrfnl jrfnl mentioned this pull request Aug 4, 2023
Open
40 tasks
@MarkMaldaba
Copy link
Copy Markdown
Contributor

MarkMaldaba commented Aug 4, 2023

This is not a BC break as the NewArrowFunctionSniff was new to PHPCompatibility 10.0.0 anyway.

I'd be a little careful with that. We've been telling people to use the develop branch for a long time, in the absence of a proper release, so I would be very wary about introducing BC breakages in the develop branch. It is likely to impact on production uses of the tool, as that's what we've been telling people to do.

@jrfnl
Copy link
Copy Markdown
Member Author

jrfnl commented Aug 4, 2023 •
edited
Loading

This is not a BC break as the NewArrowFunctionSniff was new to PHPCompatibility 10.0.0 anyway.

I'd be a little careful with that. We've been telling people to use the develop branch for a long time...

@MarkMaldaba You have point. All the same, anyone using develop should realize they run the risks of (small) BC-breaks until there is a tagged release.

I also don't think this BC break will actually have any real impact as - unlike functions/classes etc - the arrow function syntax is not something which can be polyfilled via userland code,

The only impact it could possibly have is when projects have a mix of files for different PHP versions and would have excludes in the ruleset and/or ignore annotations in those files for specific notices from PHPCompatibility (in this case about arrow functions).

Generally speaking, projects with different supported PHP versions for different files are not very common and as a general rule of thumb, they should run PHPCompatibility multiple times with different testVersions in that case, as otherwise, that list of excludes/ignores is likely to keep growing... (as anything "new" we add detection for would be a potential BC-break for those files).

@MaxwellGarceau MaxwellGarceau mentioned this pull request Jan 7, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wimg wimg wimg approved these changes

Assignees

No one assigned

Labels

Type: chores/QA

Projects

None yet

Milestone

10.0.0-alpha1

Development

Successfully merging this pull request may close these issues.

3 participants

@jrfnl @MarkMaldaba @wimg