libs/pubsub/query: specify peg version in go.mod #9099
Conversation
The code to generate the pubsub queries was dependent on an unspecified version of the peg tool. This brings peg into go.mod so it is on a fixed version. This should also enable dependabot to notify us of future updates to peg. The version of query.peg.go generated from the current version of peg correctly contains the special "Code generated by..." line to indicate to other tools that the file is automatically generated and should therefore be excluded from linters, etc. I removed the make target as there were no git grep results referencing "gen_query_parser"; directly running "go generate" is a reasonable expectation in Go projects. Now that "go run" is module aware, I would typically use "go run" inside the go:generate directive, but in this case we go build to a gitignore-d directory in order to work around the nondeterministic output detailed in pointlander/peg#129.
mark-rushakoff
requested review from
ebuchman,
cmwaters,
tychoish,
williambanfield and
creachadair
as code owners
The newly generated peg code returns an error from Init(); the previous version was niladic.
|
Alternatively you might want to consider #7338 |
|
I think #7383 is the better solution to this problem, but also this change is fine. My only concern is that we make sure that the regeneration detector triggers for this would detect if there's an upgrade to the library that causes the code to change. I sometimes see errors caused by the generated code change detector, but haven't had cause to dig into its actual operation. |
|
To be clear, I don't have any objection to this change either. It's just that we don't need to depend on code generation and we get a massive speedup and reduction in memory use with the other approach. (That said, it's the same API either way) |
|
I'm okay with the principle of bringing in the alternative implementation #7338 instead of this -- I'm inclined to say it's "safe" for 0.37, but I don't have enough context to say so confidently. I don't mind dropping this PR and porting #7338 instead, so WDYT @tychoish @creachadair? |
It's probably simpler to just merge this as-is for now. One thing I forgot is that because |
mark-rushakoff
added
the
S:automerge
tychoish
requested review from
thanethomson,
sergio-mena and
jmalicevic
as code owners
The code to generate the pubsub queries was dependent on an unspecified
version of the peg tool. This brings peg into go.mod so it is on a fixed
version. This should also enable dependabot to notify us of future
updates to peg.
The version of query.peg.go generated from the current version of peg
correctly contains the special "Code generated by..." line to indicate
to other tools that the file is automatically generated and should
therefore be excluded from linters, etc.
I removed the make target as there were no git grep results referencing
"gen_query_parser"; directly running "go generate" is a reasonable
expectation in Go projects.
Now that "go run" is module aware, I would typically use "go run" inside
the go:generate directive, but in this case we go build to a gitignore-d
directory in order to work around the nondeterministic output detailed
in pointlander/peg#129.