Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Yaml] Add --exclude and negatable --parse-tags option to lint:yaml command#39641

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@christingruber
Copy link
Contributor

@christingruberchristingruber commentedDec 28, 2020•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsNone
LicenseMIT
Doc PRsymfony/symfony-docs#14780

Added a new--exclude option to exclude one or more specific files from multiple file list:

lint:yaml dirname --exclude=/dirname/foo.yml --exclude=/dirname/bar.yml

Allow negatable for the parse tags option with--no-parse-tags and increase thesymfony/console dependency to version 5.3

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbotcarsonbot changed the titleAdds YAML lint config[Yaml] Adds YAML lint configDec 28, 2020
@jderussejderusse added this to the5.x milestoneDec 28, 2020
Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we should also add a test that handles a call of the command with a config file being passed as well as other options.

christingruber reacted with thumbs up emoji
@christingruber
Copy link
ContributorAuthor

christingruber commentedJan 5, 2021•
edited
Loading

I think we should also add a test that handles a call of the command with a config file being passed as well as other options.

I've added a simple mixed test,settings from yaml config will have priority in this case

@christingruber
Copy link
ContributorAuthor

I have changed the load order to allow overrides by console args...

@christingruber
Copy link
ContributorAuthor

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

@christingruberchristingruberforce-pushed thefeature/yaml-linter-config branch 3 times, most recently fromf09c5e1 to6a964b3CompareJanuary 14, 2021 11:27
@javiereguiluz
Copy link
Member

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 simplelint.sh orlint.bat script in thebin/ dir of the project could work too to avoid passing all the long option values. Cheers!

jvasseur reacted with thumbs up emoji

@christingruber
Copy link
ContributorAuthor

@javiereguiluz Thanks for your feedback.

No problem I think we can hold the--exclude option and drop the config part. If so, I can refactor and rebase the branch in the next days. OK?

@javiereguiluz
Copy link
Member

@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!

christingruber reacted with thumbs up emoji

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 theexclude feature.

christingruber reacted with thumbs up emoji
Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Great. Thank you for this PR.

I added some questions. Also, can you make sure the PR description is up to date? It will make it easier to understand this feature after the PR is merged.

christingruber reacted with thumbs up emoji
chalasr
chalasr approved these changesAug 9, 2021•
edited
Loading
@christingruberchristingruber changed the title[Yaml] Adds YAML lint config[Yaml] Adds ---exclude and negatable --parse-tags option to lint:yaml commandAug 9, 2021
@christingruberchristingruber changed the title[Yaml] Adds ---exclude and negatable --parse-tags option to lint:yaml command[Yaml] Add exclude and negatable parse-tags option to lint:yaml commandAug 9, 2021
@christingruberchristingruber changed the title[Yaml] Add exclude and negatable parse-tags option to lint:yaml command[Yaml] Add --exclude and negatable --parse-tags option to lint:yaml commandAug 9, 2021
@christingruber
Copy link
ContributorAuthor

I added some questions. Also, can you make sure the PR description is up to date?...

Hi@Nyholm,

thanks for your Feedback, I updated the MR description / changelog and the composer deps. (See my further comments)

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you.

I like this feature and the implementation. I also appreciate that you updated the PR description and that you prepared a doc PR.

Well done.

christingruber and OskarStark reacted with thumbs up emoji
Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

One minor

@christingruberchristingruberforce-pushed thefeature/yaml-linter-config branch 3 times, most recently from2f64ce5 to312b6f2CompareAugust 14, 2021 18:33
@christingruberchristingruberforce-pushed thefeature/yaml-linter-config branch 3 times, most recently from3cc8818 to5a37229CompareAugust 14, 2021 20:28
@chalasrchalasrforce-pushed thefeature/yaml-linter-config branch from5a37229 to55704f3CompareAugust 14, 2021 21:44
@chalasr
Copy link
Member

Thank you@christingruber.

@chalasrchalasr merged commita9753d7 intosymfony:5.4Aug 14, 2021
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull requestSep 23, 2021
…on (christingruber)This PR was merged into the 5.4 branch.Discussion----------[Yaml] Adds chapters for YAML lint --exclude optionAdds the new chapters for the yaml lint ``--exclude`` and ``--config`` options.See the feature pull:symfony/symfony#39641Commits-------9b0fe51 Add the docs for the new --exclude option
This was referencedNov 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@OskarStarkOskarStarkOskarStark approved these changes

@NyholmNyholmNyholm approved these changes

@chalasrchalasrchalasr approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

@stofstofAwaiting requested review from stof

@jderussejderusseAwaiting requested review from jderusse

+1 more reviewer

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

12 participants

@christingruber@carsonbot@javiereguiluz@chalasr@OskarStark@fabpot@stof@jderusse@Nyholm@xabbuh@ogizanagi@derrabus
[8]ページ先頭
©2009-2025 Movatter.jp