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

feat: add the ability to ignored keys while sorting #689

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fernandezcuesta
Copy link

@fernandezcuesta fernandezcuesta commented Sep 23, 2024

Hi, the use case here is to allow nested maps such as:

versions:
  - name: v1alpha1
    capabilities: [ ... ]
    customField: ""
  - name: v1alpha2
    customField: ""

not to be sorted, since there are times when we consider a primary/merge-key useful to remain wherever we define it (usually in 1st place)

@coveralls
Copy link

coveralls commented Sep 23, 2024

Coverage Status

coverage: 99.825%. remained the same
when pulling 1346875 on fernandezcuesta:master
into 8513d9b on adrienverge:master.

Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

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

Hello J.M., thanks for contributing!

The idea looks good. I propose these modifications:

  • Name the new option ignored-keys for explicitness (if we add other "ignored" options in the future).
  • Accept regexes instead of fixed strings, for more powerful usage. See for example the rule quoted-strings with its options extra-required and extra-allowed.

Also, can you include more tests? Especially for more than 1 ignored-keys, and with the regex feature we should also check it, e.g. name, aaa name and name zzz for ignored-keys: ['name'] vs. ignored-keys: ['^name$'].

Finally, you need to add a .. rubric:: Options in documentation (see other rules for examples).

@@ -88,7 +98,8 @@

ID = 'key-ordering'
TYPE = 'token'

Copy link
Owner

Choose a reason for hiding this comment

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

Don't delete this line

@fearphage
Copy link

I'm very motivated to get this merged and I see @fernandezcuesta hasn't responded in over a month. Would any be opposed to me making the reviewer comment changes in a new PR? /cc @adrienverge

FYI my use case is GitHub Actions. Sorting some of the keys there makes things more confusing/harder to maintain for humans (read: bad).

@fernandezcuesta
Copy link
Author

I'll do it, apologies for the long delay

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

Successfully merging this pull request may close these issues.

4 participants