Conversation
a1fbfa1 to
ed02781
Compare
| ) | ||
|
|
||
| // draftTags is the set of fully-qualified extension names that mark a | ||
| // declaration as experimental/draft. |
There was a problem hiding this comment.
nit: we should align on "experimental" or "draft" and not use them interchangeably. I think "draft" makes most sense as we already use "experimental" in stable API comments.
There was a problem hiding this comment.
This also applies to the proto file name.
|
|
||
| package temporal.api.batch.v1; | ||
|
|
||
| option go_package = "go.temporal.io/api/batch/v1;batch"; |
There was a problem hiding this comment.
Given there's some churn in these files (e.g. moved options, changed indentation), why don't we land a PR before any of this to only format these files the same way already? That makes the change here much smaller. If I see this right, a simple buf format -w should do the trick. That also ensures that both set of files (api and api_next) look the same structurally; it'd be odd if they didn't.
| session := &ir.Session{} | ||
| opener := baseOpener(root) | ||
|
|
||
| // first, lower the api_next set into ir |
There was a problem hiding this comment.
nit: IMO it's a good habit to write all comments as full sentences ie capitalized and with .. For consistency.
| @@ -0,0 +1,762 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
nit: calling out the obvious but still calling it out; these files should me "moved" as far as git must be concerned.
|
|
||
| stableOrder := make([]string, 0, len(paths)) | ||
| protoTexts := make(map[string]string, len(paths)) | ||
| for p, irFile := range byPath { |
There was a problem hiding this comment.
do we need stableOrder? I'd simplify it and only return protoTexts and use paths := slices.Sorted(maps.Keys(texts)) as needed. Less efficient; but that doesn't matter here and it's simpler.
| return writeFiles(outDir, stableProtos) | ||
| } | ||
|
|
||
| func addGeneratedHeader(protos map[string]string) { |
There was a problem hiding this comment.
nit: personally; I'd inline a function like this (ie both fairly short and only used once) as I find it easier to read from top to bottom than jump back and forth between function calls.
| } | ||
|
|
||
| // re-lower one more time to ensure there are no compilation issues | ||
| func verifyGeneratedProtos(ctx context.Context, root string, stableTexts map[string]string) error { |
| // of the disk corpus and WKTs. Used by stripUnusedImports (re-lowering | ||
| // draft-stripped text) and verifyStrippedProtos (combined cross-file | ||
| // verification of the final stable tree). |
There was a problem hiding this comment.
nit: I strip the used by ... part as (a) it's easy to lookup where a function is used and (b) it's easy to become stale
| relowerQueries := make([]incremental.Query[*ir.File], len(order)) | ||
| for i, p := range order { | ||
| relowerQueries[i] = queries.IR{Opener: opener, Session: session, Path: p} | ||
| } | ||
| results, err := lowerFiles(ctx, relowerQueries...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("re-lower draft-stripped corpus: %w", err) | ||
| } |
There was a problem hiding this comment.
There might be a helper opportunity here; I see three places create a session, construct queries, lower.
| if err != nil { | ||
| return fmt.Errorf("verify stable tree failed: %w", err) | ||
| } | ||
| var jErr error |
2 participants
What changed?
Why?
Breaking changes
Server PR