Comments (10)
ola-rozenfeld
commented on September 24, 2024
I agree that in C++ this is very annoyingly confusing. Moreover, the same problem exists in Python and Go as well, only Java does it right. But I think we should either make this change for all the enums on this proto, or none. The same problem applies to ExecuteOperationMetadata.Stage and DigestFunction, although to a lesser degree because those ones have more self-explanatory names.
Another good thing about your proposal is that it is not a breaking change. I mean, sure, some code would need to change, but it could be done independently in all clients and servers, and the wire format is the same.
So I like it -- I would only switch the name Enum for Value, just in case (don't want to use reserved words).
@bergsieker WDYT?
from remote-apis.
ulfjack
commented on September 24, 2024
It'll make Java code more annoying, though. Maybe this should be filed against protobuf?
from remote-apis.
jmillikin-stripe
commented on September 24, 2024
@ulfjack I expect there's a zero percent chance of upstream protobuf changing its enum scoping semantics at this point. That ship sailed ten years ago.
from remote-apis.
ola-rozenfeld
commented on September 24, 2024
Agreed. Also, as annoying goes, I much prefer annoyingly slightly more verbose (SymlinkAbsolutePathStrategy.Value.ALLOWED) to annoyingly misleading (CacheCapabilities::ALLOWED).
from remote-apis.
ulfjack
commented on September 24, 2024
It might still be worth letting them know. They did some rather large changes for proto3, and they might be thinking about this already.
from remote-apis.
ola-rozenfeld
commented on September 24, 2024
Do you prefer that we make this change now, and fix the compile of existing clients/servers, or postpone till the next API version?
I'd prefer to just do it.
from remote-apis.
jmillikin-stripe
commented on September 24, 2024
IMO: just do it. This change is backwards-compatible on the wire, and waiting longer only increases the number of clients that would need to update.
from remote-apis.
buchgr
commented on September 24, 2024
Did anyone reach out to protobuf in the meantime? @jmillikin-stripe is your proposal a recommended / best practice solution?
from remote-apis.
ola-rozenfeld
commented on September 24, 2024
I haven't, but I did an internal code search and this looks like a common
practice internally. However, very recently internally they had this exact
discussion and added a recommendation to the style guide that suggests
prefixing the enum values with the enum name, e.g:
enum DayOfWeek {
DAY_OF_WEEK_SUNDAY = 1;
DAY_OF_WEEK_MONDAY = 2;
...
}
I honestly find this pretty bad, and would much rather do the message
wrapping for us, though. But if people strongly prefer the prefix solution,
we can do that instead. Thoughts?
…On Tue, Jun 4, 2019 at 9:55 AM Jakob Buchgraber ***@***.***> wrote:
Did anyone reach out to protobuf in the meantime? @jmillikin-stripe
<https://github.com/jmillikin-stripe> is your proposal a recommended /
best practice solution?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#56?email_source=notifications&email_token=AGFAVODR52QZDS2I3NEOY5DPYZX4LA5CNFSM4GWTBV7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW4URFQ#issuecomment-498682006>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGFAVOHKFHJSYHCD2ASRJR3PYZX4LANCNFSM4GWTBV7A>
.
from remote-apis.
jmillikin-stripe
commented on September 24, 2024
The prefix solution is the traditional way, used since proto1 to match C++ style. Wrapping enums in a message only started once protobuf was ported to languages where enum value symbols are namespaced under the type.
My preference is for the wrapper message because LONG_SCREAMING_SNAKE_CASE doesn't match contemporary style in any major language. Even C++ has namespaced enum value symbols nowadays.
from remote-apis.
Related Issues (20)
- REv3 idea: Make is_topologically_sorted the default, and eliminate tag bytes
- Let exit_code be better aligned with C/POSIX
- REv3 idea: Make use of digest_function in requests mandatory
- REv3: Use IPLD (CIDs, DAG-PB, etc.)
- Chyba
- CAS: Existence Caching in Intermediate Caches (user experience report) HOT 2
- Please tag REv2 2.1.0 2.2.0 [...] HOT 6
- API extension for Git hashes HOT 1
- Googleapis is outdated HOT 1
- Should we make a resolution to NOT have a v3? HOT 2
- Support compression with external dictionary HOT 6
- Add supported_max_cas_entry_size property to CacheCapabilities HOT 2
- Bazel version to use to run hooks/pre-commit unclear HOT 1
- REv3: Reduce asymmetry between O(n) output files and O(1) output directories HOT 2
- Platform standardization HOT 1
- [Discussion] Make CAS blobs tied to ActionKey to improve sharding locality HOT 12
- Cache Capability to indicate that CAS is read-only HOT 2
- Remote Asset API: clarification about Qualifier HOT 2
- Allow abitrary tagging in RequestMetadata
- Support range downloads in the Remote Asset API HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from remote-apis.