-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Comments
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.
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 If you check the other tooling, their implementations have conflicting precedence rules.
Some other tools:
|
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). |
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.
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.
Support for 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 |
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) |
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.
Appreciated. For the record/note to self: this would also need updates in the wiki (which is not publicly editable).
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.
Appreciated. |
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 usingdist
files. Next files withdist
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?
The text was updated successfully, but these errors were encountered: