format: Bracketing keyword ref elements in formatter output #7010
Conversation
Signed-off-by: Johan Fylling <[email protected]>
Signed-off-by: Johan Fylling <[email protected]>
| @@ -0,0 +1,11 @@ | |||
| package test.contains2 # FIXME: refs aren't allowed to contains keywords | |||
There was a problem hiding this comment.
Turns out, not allowing keywords in refs is an actual issue in v1, and not just a nuisance.
We can make the test.contains package parse by bracketing it: test["contains"].
This is however not only ugly for no actual benefit, but the formatter will actually output it as package test.contains, which won't parse 😞.
We could fix the formatter to output package test["contains"] instead. But I think we should instead allow refs to contain keywords.
There was a problem hiding this comment.
But I think we should instead allow refs to contain keywords.
Was there a reason to not allow refs to contain keywords?
We could fix the formatter to output package test["contains"]
We could update the --rego-v1 flag to do that. But as you said we should allow refs unless there is a good reason not to do it.
There was a problem hiding this comment.
I think the argument against allowing keywords in refs is that we don't allow such rule names, e.g.
package foo
if if { true } # error: 'if' is not a valid rule namebut we allow you to create an empty module:
package foo.ifwhich would generate an empty virtual document at data.foo.if. This creates a sort of parity mismatch, where we allow you to create data.foo.if one way, but not the other.
There was a problem hiding this comment.
That can still be created via JSON data, so won't that mismatch always exist? The question is why you wouldn't be allowed to reference such data (without special handling / quoting) where it's impossible for it to clash with a keyword... i.e. input.package is obviously distinctly different from package alone.
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Johan Fylling <[email protected]>
johanfylling
force-pushed
the
rego-v1/update_format_tests
branch
from
5f8cb6c
to
71eb556
Compare
johanfylling
changed the title
format pkg tests to be 1.0 compatible
format/format.go
Outdated
| @@ -1067,7 +1089,8 @@ func (w *writer) writeImports(imports []*ast.Import, comments []*ast.Comment) [] | |||
| }) | |||
| for _, i := range group { | |||
| w.startLine() | |||
| w.write(i.String()) | |||
| //w.write(i.String()) | |||
| Path: ast.MustParseTerm("future.keywords." + kw), | ||
| // NOTE: This is a hack to not error on the ref containing a keyword already present in v1. | ||
| // A cleaner solution would be to instead allow refs to contain keyword terms. | ||
| Path: ast.MustParseTerm("future.keywords[\"" + kw + "\"]"), |
There was a problem hiding this comment.
I think adding an example in the comment would be helpful.
| @@ -7,3 +7,7 @@ contains := 2 | |||
| in := 3 | |||
|
|
|||
| every := 4 | |||
|
|
|||
| p { | |||
| data.foo.contains.bar == 42 | |||
There was a problem hiding this comment.
So this won't be formatted to data.foo["contains"]["bar"] since the foo is not a keyword?
There was a problem hiding this comment.
It would have been converted to data.foo["contains"].bar, but other parts of this module are triggering errors.
| @@ -0,0 +1,13 @@ | |||
| package test["contains"] | |||
There was a problem hiding this comment.
Maybe change this to package test.contains and the formatter makes it test["contains"]
There was a problem hiding this comment.
Can't do that, as the parser is operating under v1 rules and would reject that package ref.
There was a problem hiding this comment.
So package test.contains will be a parser error in v1 but when formatted using --rego-v1 flag, the path will get converted to package test["contains"]
| @@ -0,0 +1,10 @@ | |||
| package test["if"] | |||
There was a problem hiding this comment.
Maybe change this to package test.if and the formatter makes it test["if"]
Signed-off-by: Johan Fylling <[email protected]>
Also future-proofing
formatpkg tests to be 1.0 compatible.