Remove jsonOptions from AST nodes and terms #7281
Conversation
anderseknert
force-pushed
the
no-json-options
branch
from
165ffd4 to
c7ff0d7
Compare
anderseknert
force-pushed
the
no-json-options
branch
from
c7ff0d7 to
c7c4fab
Compare
| lock sync.RWMutex | ||
| } | ||
|
|
||
| var options = &configuredJSONOptions{ |
There was a problem hiding this comment.
Is there a reason why this is a pointer?
There was a problem hiding this comment.
The lock on the struct. If the struct isn't a pointer then the lock would have to be, and the "result" would be the same more or less.
| astJSON.SetOptions(astJSON.Options{ | ||
| MarshalOptions: astJSON.MarshalOptions{ | ||
| IncludeLocation: astJSON.NodeToggle{ | ||
| // Annotation location data is only included if includeAnnotations is set |
There was a problem hiding this comment.
Yes, thanks, I'll fix that.
| AnnotationsRef: true, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Clearly, this is needed to get the test to pass, but I'm missing where this defaulted to true before.
There was a problem hiding this comment.
The true in File(rootDir, true) is for including annotations. I had put a SetOptions call inside of that loader but removed it now.. this should only have impact on serialization.
| } | ||
|
|
||
| // GetOptions returns (a copy of) the global options for marshalling AST nodes to JSON | ||
| func GetOptions() Options { |
There was a problem hiding this comment.
Consider renaming these to something like GlobalOptions() and SetGlobalOptions() to hammer the "global" part home.
If options, above, doesn't need to be a pointer, we could even make that a public GlobalOptions var and attach these functions as methods?
There was a problem hiding this comment.
I guess it's just a matter of preference, but to me, astJSON.GlobalOptions.Set() looks slightly worse 😅 I think I went with this naming mostly to try and follow the previous convention. I don't feel strongly about it, and even less so considering this mostly will go unused :)
| foo := "bar" | ||
| ` | ||
| assertParseModuleJSONOptions(t, `foo := "bar"`, module, parserOpts) | ||
| } |
There was a problem hiding this comment.
There is no point in keeping these tests but using the global options instead?
There was a problem hiding this comment.
Not really — these tests assert that the parser specifically would use the options it got to propagate them on each node parsed. Since we use a global option now, the parser doesn't need to be involved at all in this.
| func VarHead(name Var, location *Location, jsonOpts *astJSON.Options) *Head { | ||
| // VarHead creates a head object, initializes its Name and Location and returns the new head. | ||
| // NOTE: The JSON options argument is no longer used, and kept only for backwards compatibility. | ||
| func VarHead(name Var, location *Location, _ *astJSON.Options) *Head { |
There was a problem hiding this comment.
This might go against what might be otherwise be considered idiomatic go, but we could also make the *astJSON.Options arg variadic, so new code isn't required to use it, and maybe make it easier to drop in the future.
There was a problem hiding this comment.
Tbh, I'd be more inclined to just drop the last argument directly. Even we don't use this ever outside of the parser, so it's hard to think there's external code depending on this... but i guess we can do that if/when we drop this altogether.
anderseknert
force-pushed
the
no-json-options
branch
from
c7c4fab to
0f66559
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
anderseknert
force-pushed
the
no-json-options
branch
from
0f66559 to
1e5d054
Compare
This was originally added to allow Regal to get a serialized AST that included location data wherever that was possible. Regal is however no longer using OPA's JSON serialization but its own custom encoder. Attaching these options to every AST node and term comes with a cost attached, and without any known users of this feature vs. the many users who care about resource utilization, this feels like an easy choice. While it seems unlikely to be users depending on this functionality — in case someone needs it, the options for serializing AST nodes to JSON can now be set globally instead. Global state is always awkward, but since JSON marshalling methods only have access to the node being marshalled and of course, global state, there's not a whole lot of options if we intend to keep this feature. Signed-off-by: Anders Eknert <[email protected]>
anderseknert
force-pushed
the
no-json-options
branch
from
1e5d054 to
10b4bc4
Compare
This was originally added to allow Regal to get a serialized AST that included location data wherever that was possible. Regal is however no longer using OPA's JSON serialization but its own custom encoder. Attaching these options to every AST node and term comes with a cost attached, and without any known users of this feature vs. the many users who care about resource utilization, this feels like an easy choice.
While it seems unlikely to be users depending on this functionality — in case someone needs it, the options for serializing AST nodes to JSON can now be set globally instead. Global state is always awkward, but since JSON marshalling methods only have access to the node being marshalled and of course, global state, there's not a whole lot of options if we intend to keep this feature.