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

Support dist config file name phpcs.dist.xml #770

Open
2 tasks done
martinssipenko opened this issue Dec 12, 2024 · 5 comments
Open
2 tasks done

Support dist config file name phpcs.dist.xml #770

martinssipenko opened this issue Dec 12, 2024 · 5 comments

Comments

@martinssipenko
Copy link
Contributor

Describe the solution you'd like

I'd like to re-open a discussion that was started in #371.

The reasoning for having this would be that this would allow consistency in terms of the naming of config files across various QA tooling commonly used by PHP projects. For example, PHPUnit, PHPStan, and Psalm all support config file names that conform <tool name>.dist.<file ext> scheme.

While using --standard=phpcs.dist.xml is indeed an option, it requires developers to use the flag as they run PHPCS, which is an extra thing they have to remember to do and extra characters they must type. IMO it would be a better developer experience if they did not have to do it.

Overall, I see that this is a very tiny thing that borderlines with some people's OCD (mine 😂) of wanting to have a consistent PHP tool config file naming strategy.

Regarding file precedence - in my understanding phpcs should first look for files that don't have dist in them as those could be used be developers to override the "default" projects config distributed using dist files. Next files with dist should be loaded, the order of each particular type of dist file name doesn't matter as projects very likely will not have various favours of dist and non-dist config files. In other words, if non-dist file is present then use then, if not fall back to dist.

Regarding releasing it as part of 4.0 - why do you think this addition should be considered a BC breaking change?

@jrfnl
Copy link
Member

jrfnl commented Dec 12, 2024

Historic reference: squizlabs/PHP_CodeSniffer#3744

@martinssipenko While I appreciate the interest in this feature, I'm still not convinced and have seen very little real interest in the feature over time.

What other tools do is still not an argument in favour of anything (and I've said so before). That would only be a valid argument if those projects, including PHPCS, would come together and discuss and agree on standards for these type of things at some point.

PHPCS already supports four different names recognized by default and allows for custom names too, which is more than any of these other tools.

Adding support for yet more, is just increasing the maintenance burden for little benefit.

Regarding file precedence - in my understanding phpcs should first look for files that don't have dist in them as those could be used be developers to override the "default" projects config distributed using dist files. Next files with dist should be loaded, the order of each particular type of dist file name doesn't matter as projects very likely will not have various favours of dist and non-dist config files. In other words, if non-dist file is present then use then, if not fall back to dist.

This doesn't answer the question of file precedence at all and contains unvalidated assumptions (which will always come back to bite you).

The question is whether phpcs.dist.xml should have precedence over phpcs.xml.dist and how the file precedence with the . prefixed versions of the files should be.

If you check the other tooling, their implementations have conflicting precedence rules.

  • PHPUnit: dist.xml takes precedence over xml.dist.
  • PHPStan: neon.dist takes precedence over dist.neon.

Some other tools:

  • PHPDocumentor: dist.xml takes precedence over xml.dist.
  • Psalm: xml.dist takes precedence over dist.xml.

@martinssipenko
Copy link
Contributor Author

I get that what others tools do doesn’t matter in context of this tool, however if we look at php ecosystem as whole having a somwaht similar approach would be awesome thing to have. I say this as a person who works on several php projects at a time, and having higher cosistency is something I would find appealing.

Regarding precedence, my thinking is that it does not matter which dist files would be loaded first as projects likley wont have two dist files. However, to reduce potential issues we could say dist.xml is loaded last, as it would be a new addition, so projects that have xml.dist dont get affected by this change. In other words xml.dist wins over dist.xml if both exist. Regarding dot prefix, i’d use same order as currently supported config files have, that is one with dot wins over one without it (if I understand code correctly, have not tried it by running phpcs with several files present).

Regarding maintenace burden, that is hard for me to judge as I’m not a maintainer of the project, however, from my subjective pov this is very little addition and should not cause too much trouble. I’d be happy to contribute the changes required for this, including tests and documentation.

I would also propose that support for file names with dot prefix would get removed in 4.0 as this would be a BC break. That would also remove the amount of supported file names from current 4 to 3 (assiming dist.xml is implemented).

@jrfnl
Copy link
Member

jrfnl commented Dec 12, 2024

I get that what others tools do doesn’t matter in context of this tool, however if we look at php ecosystem as whole having a somwaht similar approach would be awesome thing to have. I say this as a person who works on several php projects at a time, and having higher cosistency is something I would find appealing.

As I stated before, I understand the desire behind this FR, but the reality is that things are not consistent no matter what and that's not going to change without the tooling maintainers coming together and agreeing on some standards, which hasn't happened, so trying to "force" your idea of consistency on projects without the implementations actually being consistent is just making things worse IMO.

Regarding precedence, my thinking is that it does not matter which dist files would be loaded first as projects likley wont have two dist files.

That is a logical fallacy and not acceptable. Anything users can do, they will do, so making assumptions like that will just lead to bad code and increase the support burden.

Any addition to the accepted config files would have to have a clearly documented and consistent precedence order.

I would also propose that support for file names with dot prefix would get removed in 4.0 as this would be a BC break. That would also remove the amount of supported file names from current 4 to 3 (assiming dist.xml is implemented).

Support for . prefixed config files was added via squizlabs/PHP_CodeSniffer#1566.

I hope you are joking ? Why would your "OCD"-like preference (your terminology) for how files are named be satisfied, while you want to disregard and disable the same for others ? That kind of attitude makes me even more reluctant to accept this feature request.

Either way that should be a separate discussion. (Oh and at least the . prefixed addition had good arguments behind it)

@martinssipenko
Copy link
Contributor Author

Please disregard my previous comment about dot prefixed files. I now better understand the situation and you are right, removing it would indeed hurt others which is not at all I’d like to suggest.

Going back to percedence, excuse me for my pushiness, but perhaps I’m just not seeing this as you are as I have much more limited context and scope in this. Wouldn’t adding new option as last in precedence be least harmful in backwards compatability? Regarding documenation, I 100% agree and am commited to doing it via PRs if there is an agreement that this new file name should be supported.

End of the day its your call as maintainer wether to have such feature or not. I don’t want to push anything on you as I’m sure there are bigger fish to fry in context of this project, and if this requires unreasonable effort from your side, lets just close this. From my side, I’d love if this would be possible, I checked the PRs and issues to make sure I dont reopen a closed and discussed topic, I found the PR mentioned which appeared to me as closed due to no response from the person that raised it, therefore without a closing resolution, so I decided to reopen it as a another +1 for FR.

In closing, I want to say that I find it amazing that you have put your effort into reviving this project, I really do admire it. Thank you for doing that! ❤️ (please do not interpret this thank you as me trying to convince you, I genually just want to say thanks)

@jrfnl
Copy link
Member

jrfnl commented Dec 12, 2024

@martinssipenko

Going back to percedence, excuse me for my pushiness, but perhaps I’m just not seeing this as you are as I have much more limited context and scope in this. Wouldn’t adding new option as last in precedence be least harmful in backwards compatability?

Regarding precedence: making the precedence order explicit (i.e. show me a list) and agreed upon will be a prerequisite for any PR.

Note: I'm not saying I disagree with your current proposal, but I want to make sure there is no room for interpretation/misunderstandings about the precedence order.

Regarding documenation, I 100% agree and am commited to doing it via PRs if there is an agreement that this new file name should be supported.

Appreciated. For the record/note to self: this would also need updates in the wiki (which is not publicly editable).

End of the day its your call as maintainer wether to have such feature or not. I don’t want to push anything on you as I’m sure there are bigger fish to fry in context of this project, and if this requires unreasonable effort from your side, lets just close this.

I've already added the "waiting for opinions" label. At this time, I see this as a niche request with very few supporters (based on the historic tickets), so I'm doubtful the benefits outweight the cost.

Having said that, if it would turn out that this is something desired by substantially more users, I'd be open to accepting a PR, so I suggest we leave the ticket open for a while to see if it gathers more support.

In closing, I want to say that I find it amazing that you have put your effort into reviving this project, I really do admire it. Thank you for doing that! ❤️ (please do not interpret this thank you as me trying to convince you, I genually just want to say thanks)

Appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants