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

is our block description syntax valid yaml? #312

Closed
williballenthin opened this issue Sep 12, 2020 · 7 comments · Fixed by #327
Closed

is our block description syntax valid yaml? #312

williballenthin opened this issue Sep 12, 2020 · 7 comments · Fixed by #327
Assignees
Labels
bug Something isn't working

Comments

@williballenthin
Copy link
Collaborator

williballenthin commented Sep 12, 2020

currently, we allow rule authors to add a description to and/or/etc. like so:

and:
  - match: foo
  - match: bar
  description: this is awesome!

(concrete example here)

this seemed like a natural way to extend our two-line syntax for feature descriptions, which look like:

and:
  - string: foo
    description: bar

however, i'm not sure that the former is valid yaml. ruamel accepts it, but pyyaml does not. take a moment to look at it and try to figure out how it should be parsed? is the child of the and a list or dictionary???

@williballenthin williballenthin added the bug Something isn't working label Sep 12, 2020
@williballenthin
Copy link
Collaborator Author

one way to make this work while only slightly tweaking the syntax is to make description an element in the list, like:

and:
  - match: foo
  - match: bar
  - description: this is awesome!

@williballenthin
Copy link
Collaborator Author

downside: fixing up this syntax is technically a breaking change, affecting the last minor version or so. the alternative is to suffer having rules that are maybe not valid yaml (something I think is fairly important).

personally, I probably prefer to keep things as we intended and make the change (violating semver and assuming no one will notice).

@williballenthin
Copy link
Collaborator Author

upside: the description can now be placed at the top of a block, like:

and:
  - description: everything is awesome!
  - match: foo
  - match: bar

I think all our rules with the conflicting syntax have the description as the last element, which I personally find hard to follow, all because this is all that ruamel accepted.

@williballenthin
Copy link
Collaborator Author

maybe for v1.3.0 we should comment out these description lines as they currently exist so we can fully discuss the changes here while minimizing the time the invalid syntax exists.

@williballenthin
Copy link
Collaborator Author

tagging @re-fox since I know they have used this syntax to make rules easier to read

@mr-tz
Copy link
Collaborator

mr-tz commented Sep 14, 2020

Yeah, let's fix this. Commenting out is a good start!

@re-fox
Copy link
Collaborator

re-fox commented Sep 14, 2020

I agree! That sounds like a good plan.

I remember at the time, modifying the rule several times before I found a working variation. I think that's how I ended up on.

  - api: foo
  description: bar

In my opinion it's better to have commented descriptions than none at all. Those can always be uncommented and fixed up later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants