8000 Add a non-strict Style Guide group · Issue #3 · uber/prototool · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Mar 4, 2022. It is now read-only.

Add a non-strict Style Guide group #3

Closed
bufdev opened this issue Apr 10, 2018 · 7 comments
Closed

Add a non-strict Style Guide group #3

bufdev opened this issue Apr 10, 2018 · 7 comments
Labels
feature New feature or request

Comments

@bufdev
Copy link
Contributor
bufdev commented Apr 10, 2018

We've committed our draft Style Guide into etc/style/uber/uber.proto. This is the product of a lot of hashing out for what we want Protobuf files to look like within Uber. The rules inside this are inherently strict - we want there to be One Way To Do Things™ within Uber, and we want easy-to-follow rules that prevent users from making common mistakes. As Jisi on the Protobuf team at Google pointed out though, this style guide would require migration for most existing users.

What my hope is is that we have the new "recommended/strict Style Guide" that accomplishes our goals within Uber, which I think apply to the greater community - if we have a consistent way to do enums, a consistent naming convention for go_package, java_package, etc, everyone wins, and makes things like consistent output of stubs easier. We also have the "default Style Guide" that does the bare minimum enforcement.

For the record, here are the rules I have to ignore to get googleapis to pass lint:

lint:
  exclude_ids:
    - REQUEST_RESPONSE_TYPES_UNIQUE
    - REQUEST_RESPONSE_TYPES_IN_SAME_FILE
    - ENUM_FIELD_PREFIXES
    - ENUM_ZERO_VALUES_INVALID
    - FILE_OPTIONS_EQUAL_GO_PACKAGE_PB_SUFFIX
    - FILE_OPTIONS_EQUAL_JAVA_PACKAGE_COM_PB
    - FILE_OPTIONS_REQUIRE_JAVA_PACKAGE

Action items:

  • Go over the Style Guide as proposed more, and come to a common "strict" Style Guide.
  • Figure out what belongs in the strict Style Guide vs the default Style Guide.

Copying Jisi's feedback from an email (hope this is ok Jisi!):

There are two parts in the style guide,

  • Fromatting rules., e.g. spaces, empty lines, import orders. Those can be safely applied to the whole depot without causing any issues.
  • Naming: e.g. packages, enums, etc. Those could affect the APIs.

For a global style guide, we should probably only enforce the format ones. For the naming, we can list several best-practices (we do have such a doc in google), but as protobuf is already widely used, each organization probably has its own naming style. For example, you can find Google API's style guide here. Another idea is we can have different profile for naming.

Some of the detailed feedback about the style guide:

  • package: each organization probably has its own style of naming packages. I can image the migration cost for google to a converged package style would be impractical given the # of protos we have.
  • enum default value: internally we recommend _UNSPECIFIED or _UNSET as 0. It's the proto3 design goal to not distinguish between unset and set-to-default. In current proto3 implementation, you won't be able to distinguish between the two -- has-bit is gone and setting a value to 0 is a no-op (values default to 0). The serialization will simply skip the field even if you set it to 0 value explicitly. This is also a API change and we should probably just give suggestions rather than enforce it.
@bufdev bufdev mentioned this issue Apr 10, 2018
Closed
@jzelinskie
Copy link
Contributor

I don't think it's possible to unify the protobuf community at this point. What would be realistic might be auto-detection of different "profiles" (e.g. uber, google, default, strict). I say auto-detection because like I said, I don't think it's possible to get every protobuf user to adopt an additional configuration file.

@bufdev
Copy link
Contributor Author
bufdev commented Apr 23, 2018

Hmm, any ideas on how to auto-detect? Trying to think of a simple heuristic or set of heuristics that would work. Maybe see what fails and choose?

For Uber that would be problematic, we have to enforce the stronger one, so letting people fall back based on failures defeats the point.

@bufdev bufdev mentioned this issue Jun 26, 2018
Closed
@bufdev
Copy link
Contributor Author
bufdev commented Jul 17, 2018

@amckinney @AlexAPB I think what is needed for v1.0 is that we commit to the "strict" style guide, and provide a fallback "non-strict" style guide post-v1.0.

@peats-bond
Copy link
Contributor

Agreed, let's stay as opinionated as possible for 1.0.

@bufdev
Copy link
Contributor Author
bufdev commented Jul 31, 2018

Removing v1.0 label as this issue is more than just go over the Style Guide. However, going over the Style Guide is probably the last remaining item before v1.0.

@bufdev bufdev changed the title Agree on Style Guide/Perhaps split into "default" and "strict" mode Add a non-strict Style Guide group Aug 3, 2018
@bufdev bufdev added the feature New feature or request label Aug 3, 2018
@amckinney
Copy link
Contributor

Now that we've removed the concept of lint groups, is this still applicable? As it stands today, Prototool comes configured with a set of default linters (that happen to conform to Uber's needs). If users disagree with the defaults, they can simply configure their linters otherwise, ie

lint:
  rules:
    no_default: true
    add:
      - ...
      - ...

It doesn't seem like we should concern ourselves with other group configurations - they can be easily configured explicitly. There are an infinite number of opinions with regard to style, so I think that it's important to draw the line here.

@peter-edge Thoughts? Should we close this task?

@bufdev
Copy link
Contributor Author
bufdev commented Aug 3, 2018 •
edited
Loading

95% of the Protobuf community will not use this for linting because we don’t have an easy way to do this without listing linters. Having more users means more valid bug reports, which means better tooling for Uber.

@bufdev bufdev mentioned this issue Dec 23, 2018
Merged
sevennt pushed a commit to sevennt/prototool that referenced this issue Aug 30, 2021
@wujunze
Merge pull request uber#3 from wujunze/feature-panda-goreleaser
7bf1705 
feat: fix github action conf
sevennt pushed a commit to sevennt/prototool that referenced this issue Aug 30, 2021
@wujunze
Merge pull request uber#3 from gotomicro/feature-panda-goreleaser
20a1947 
feat: fix github action conf
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jzelinskie @peats-bond @bufdev @amckinney
0