[Yaml] Add --exclude and negatable --parse-tags option to lint:yaml command #39641
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
d73e797 to
3bcfd1d
Compare
1980bd0 to
c8f8840
Compare
I've added a simple mixed test, |
1ca3b9b to
85a507f
Compare
|
I have changed the load order to allow overrides by console args... |
85a507f to
3609488
Compare
|
A short overview for the last changes: I have added some tests which handles mixed arguments from config and options from console. I have also changed the load order to allow overrides by console options. With that it is possible to override the values provided from lint config by console options. @xabbuh |
f09c5e1 to
6a964b3
Compare
6a964b3 to
cbdda1f
Compare
|
I'm divided about this proposal. I love the new options that you added to the command but I'm not sure about providing support for a new config file specifically to run this command. Just asking: if we keep all the new command options, could a makefile task be an alternative to this config file? You would add the values of all the command options to create a very long command ... but since it's a Makefile, you don't have to actually type or remember the long command, only a short alias. Maybe it's not possible, I don't know. As a poor version of the Makefile, a simple |
|
@javiereguiluz Thanks for your feedback. No problem I think we can hold the |
|
@christingruber thanks ... but maybe we should wait for other comments from @symfony/mergers first. The reason is that in my previous comment I only gave my opinion ... and a single person opinion is not important. Better see what's the general consensus about what to do here. Thanks! |
There was a problem hiding this comment.
Wow, what a great PR. Well done. I like that you added tests and a good description in the changelog. I also like the implementation.
I do agree with @javiereguiluz, Let's not add a config file. If we would go the config file route, we must also support different formats and it would make sense to allow other commands to have config files too. It will be a large feature and Im not sure about that route. I think the workarounds mentioned before is better.
I would be happy to see this PR with only the exclude feature.
345671e to
ca1608e
Compare
Hi @Nyholm, thanks for your Feedback, I updated the MR description / changelog and the composer deps. (See my further comments) |
2f64ce5 to
312b6f2
Compare
3cc8818 to
5a37229
Compare
5a37229 to
55704f3
Compare
|
Thank you @christingruber. |
…on (christingruber) This PR was merged into the 5.4 branch. Discussion ---------- [Yaml] Adds chapters for YAML lint --exclude option Adds the new chapters for the yaml lint ``--exclude`` and ``--config`` options. See the feature pull: symfony/symfony#39641 Commits ------- 9b0fe51 Add the docs for the new --exclude option
Added a new
--excludeoption to exclude one or more specific files from multiple file list:lint:yaml dirname --exclude=/dirname/foo.yml --exclude=/dirname/bar.ymlAllow negatable for the parse tags option with
--no-parse-tagsand increase thesymfony/consoledependency to version 5.3