Closed (fixed)
Project:
Coder
Version:
8.3.x-dev
Component:
Coder Sniffer
Priority:
Major
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
23 Oct 2024 at 23:21 UTC
Updated:
13 Dec 2024 at 19:04 UTC
Jump to comment: Most recent
It turns out that enforcing sorting is made difficult because of the behavior of IDEs.
For example, PHPStorm has a 'sort lines' action which will sort text lines alphabetically. case sensitve. PHPStorm will also automatically add 'use' statements but in the case the sort is not case sensitive alphabetical. So this becomes a burden. for project.
Remove the use-by order rule
Allow project to opt in to the order-by rule, with or without case sensitive sort
Comments
Comment #2
eiriksmAnd what would be the actual consequences for projects that has already changed the sort order to case sensitive. Would it have to be reverted? I guess I could figure it out somehow on my own, but currently on my phone and just wondering.
Comment #3
jonathan1055 commented[For those not on Slack, here is what I said on that thread]
I think it would be better to remove the entire Coder rule, not just the latest change which added case-sensitivity. Then when the standard is agreed, we can write the sniffs that implement exactly what we want. If we leave in place the sniff which does a plain sort, we'll be creating phpcs failures again for those projects which have just committed changes to satisfy case-sensitivity. We will create more annoyance than good, until the standard is fully settled, so best to remove the Coder rule entirely.
Comment #4
jurgenhaas@jonathan1055 I like that idea but also wonder if the sort mode couldn't be configurable on a per-project basis? But I don't know Coder and PHPCS well enough, so I might be completely off track here.
Comment #5
jonathan1055 commentedYes the case-sensitivity can be configured per project by adding the following to the project's phpcs.xml file
The latest Coder commit added
property name="caseSensitive" value="true"to the default Drupal coding standard config, so the lines above effectively revert that, but leave the original non-case-sensitive sort in place. This would be OK, but it means that(a) the project maintainer has to do this change when they have already made a change to sort the statements case-sensitively,
(b) not all projects have their own phpcs.xml if they are OK with following all Drupal default standards. So we'd be forcing a project to add a phpcs.xml file just in the interim period, before the full standard is agreed, then the file may not be needed by the project when the new Coder sniffs are implemented. We'd then be forcing the maintainer to again make a change by deleting the phpcs.xml file, or removing that caseSensitve override.
(c) The is no guarantee that the final coding standard and Coder implementation of it will use the SlevomatCodingStandard sniff. There were certainly a few bugs in the phpcbf fixer for it it when we first worked on adding that standard. It might be that we write our own sniffs in Coder. Then that projects own phpcs.xml file would be referring to a sniff that is not used, making more confusion.
All of these changes just introduce annoyance and unnecessary work, so I think the best thing to do at the moment is remove the rule entirely until the new standard is tested and written up and Coding Standards issue #1624564: Coding standards for "use" statements is fully agreed and completed.
Comment #6
cmlaraI agree, given there is no official standard at the moment and the final standard is still very much undecided removing the sniff completely appears to be the best choice.
Updating IS/Title to reflect new proposal
Comment #7
jonathan1055 commentedI have created a GitHub PR for this change
https://github.com/pfrenssen/coder/pull/240
The issue summary says "Allow project to opt in to the order-by rule, with or without case sensitive sort" but this is not a thing that Coder can do. It just needs to be documented (somewhere) that the line
<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses">can be added to the project's phpcs.xml file if they want to opt in.Comment #8
catchAgreed, better to remove this then enable it once there's a proper standard.
Comment #9
jonathan1055 commentedHi @klausi,
The priority of this is set to 'major' and it has been RTBC for a week. Any chance you could commit it and make new Coder release, please? The longer the sniff stays in the more grief we are causing for developers, until the standard has been agreed.
Thanks.
Comment #10
cmlaraAnyone heard anything from @klausi? Should we ping on Slack?
Comment #11
jonathan1055 commented@klausi is in the #coder channel (I added him :-) but the other maintainers are not in that channel. I don't think it is fair to always ping one person if there are other maintainers too, we could add them there as that would be good, but I do not know which of these are active in Coder now:
Comment #12
cmlaraGenerally fair, I will note klausi has made 100% of the releases since 8.x-3.2 in April 2019.
GitLab Activity report indicates klausi pushed all but one of the changes into the repo in the past 2 years
https://git.drupalcode.org/project/coder/activity
I believe we can call this (at least the D.O. side of the project) a 'single active maintainer' project.
Edit: I missed one push a year ago by @pfrenssen
Comment #14
arkener commentedAgreed, we should remove the sniff and revisit once #1624564: Coding standards for "use" statements is approved. I've merged the PR into 3.x, and I'll plan to create a new release this weekend, assuming no unexpected issues pop up. Thanks for the work on this!
Comment #15
jonathan1055 commentedThank you @arkener, that's great.
Comment #16
klausiSorry, was busy with other things. Thanks a lot for stepping up Arkener, love the Drupal community!
I will make a release now and explain in the release notes how people can add a snippet to their phpcs.xml file to keep sorted use statements. It is very useful in teams to better avoid git merge conflicts.
Comment #17
klausiReleased: https://www.drupal.org/project/coder/releases/8.3.26
Thanks everyone!