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

Fix name collision in program proto #3949

Merged
merged 4 commits into from
Mar 22, 2021

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Mar 21, 2021

Fix name collision in program proto around Entry-s (causes a name collision in the discovery API generation in g3).

@balopat balopat requested review from cduck, vtomole, wcourtney and a team as code owners March 21, 2021 22:34
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Mar 21, 2021
Copy link
Collaborator

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO(95-martin-orion): Update #3923 with these changes.

@@ -276,25 +276,25 @@ message MeasurementKey {
// swap measurement keys a and b.
message MeasurementKeyMapping {
// Indicates that measurement key "key" should be replaced with "value".
message Entry {
message MeasurementKeyEntry {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, the other two should be QubitEntry and ArgEntry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point with the consistency - I would call this instead MeasurementKeyMappingEntry then...? Too long?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for QubitEntry, ArgEntry, MeasurementKeyEntry.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing @maffoo: these will always appear as *Mapping.<name>. We only need enough prefix to dedupe these types, so I also prefer *Entry instead of *MappingEntry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, fixing!

@maffoo
Copy link
Contributor

maffoo commented Mar 22, 2021

Too bad that discovery api generation doesn't respect nested messages. This would seem to me to be a common problem; is there any way to change the discovery API generation to deal with this?

@95-martin-orion
Copy link
Collaborator

Too bad that discovery api generation doesn't respect nested messages. This would seem to me to be a common problem; is there any way to change the discovery API generation to deal with this?

IIRC, there's a similar restriction on enums in protos: even when nested, they can't have the same name. I suspect this has more to do with the fundamental behavior of protos than our API generation tools.

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 22, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 22, 2021
@CirqBot CirqBot merged commit f7ccb4c into quantumlib:master Mar 22, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants