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 13 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 1efd875 on fernandezcuesta:master
into e118296 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

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 Jesús,

Thanks for the changes.

While trying to improve test coverage (see my comment below) I found a bug with this implementation: with ignored-keys: ["n(a|o)me", "^b"], it reports the line boat as an error, while it shouldn't:

a:
b:
c:
name: ignored
first-name: ignored
nome: ignored
gnomes: ignored
d:
e:
boat: ignored
.boat: ERROR
call: ERROR
f:
g:

This is because you keep adding the ignored keys to the list of previous keys to compare (context['stack'][-1].keys).

Instead of, I think your commit should simply do:

@@ -116,7 +142,9 @@ def check(conf, token, prev, next, nextnext, context):
           isinstance(next, yaml.ScalarToken)):
         # This check is done because KeyTokens can be found inside flow
         # sequences... strange, but allowed.
-        if len(context['stack']) > 0 and context['stack'][-1].type == MAP:
+        if (len(context['stack']) > 0 and context['stack'][-1].type == MAP and
+                not any(re.search(r, next.value)
+                        for r in conf['ignored-keys'])):
             if any(strcoll(next.value, key) < 0
                    for key in context['stack'][-1].keys):
                 yield LintProblem(

tests/rules/test_key_ordering.py Outdated Show resolved Hide resolved
yamllint/rules/key_ordering.py Outdated Show resolved Hide resolved
yamllint/rules/key_ordering.py Show resolved Hide resolved
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.

@fernandezcuesta it looks like you didn't check all the comments from my last review.

Would any be opposed to me making the reviewer comment changes in a new PR? /cc @adrienverge

@fearphage feel free if needed!

Comment on lines 151 to 154
if any(
strcoll(next.value, key) < 0
for key in context['stack'][-1].keys
):
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change this; and also please don't change my code proposals to use Black style: the whole codebase doesn't.

Copy link
Author

Choose a reason for hiding this comment

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

sorry for that

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