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

Fixes #971: Add a Protobuf linter to the project #1186

Merged
merged 15 commits into from
Jun 5, 2020

Conversation

anandwana001
Copy link
Contributor

Explanation

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

Overall looks good! I had a question mainly about why we are removing the except cases. Also I want to wait and see if its running okay in CircleCI before approving so let's wait on that

buf.yaml Show resolved Hide resolved
buf.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! This generally LGTM. Just had a few comments.

buf.yaml Outdated Show resolved Hide resolved
buf.yaml Show resolved Hide resolved
buf.yaml Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment May 30, 2020
@anandwana001
Copy link
Contributor Author

anandwana001 commented May 31, 2020

Screenshot 2020-06-01 at 00 25 03

Locally working fine. Need to understand the task generateProto because this is somehow connected with the domain test part and failing non_flaky due to change in proto files.

Changing import "src/main/proto/subtitled_html.proto"; to import "subtitled_html.proto";, fails
and buf says not able to find files
and reverse fails non_flaky saying generateProto fails

@anandwana001
Copy link
Contributor Author

I think it's done, I checked it over local CircleCI too.

How it seems when linters fail - changed the small c to capitalC in one of the proto file
Screenshot 2020-06-02 at 01 06 36

How it seems when linters passes
Screenshot 2020-06-02 at 01 19 41

.circleci/config.yml Outdated Show resolved Hide resolved
buf.yaml Outdated Show resolved Hide resolved
model/build.gradle Outdated Show resolved Hide resolved
buf.yaml Outdated Show resolved Hide resolved
@vinitamurthi
Copy link
Contributor

Overall its going on the right track @anandwana001 ! Just a couple things - we want to enforce the enum checks that buf is asking for, so let's not exclude that and instead make those changes in our enums. Otherwise just some nits.

Also note that the non-flaky-tests failed, but I think that's because of a circleCI load issue, can you rerun them and check?

@anandwana001
Copy link
Contributor Author

Overall its going on the right track @anandwana001 ! Just a couple things - we want to enforce the enum checks that buf is asking for, so let's not exclude that and instead make those changes in our enums. Otherwise just some nits.

Also note that the non-flaky-tests failed, but I think that's because of a circleCI load issue, can you rerun them and check?

Ok, I will rerun and check. Does modifying enums in all these proto files will affect other parts of the project?

@rt4914
Copy link
Contributor

rt4914 commented Jun 3, 2020

@rt4914 Can you resolve comments on my behalf once @anandwana001 makes the changes?

@vinitamurthi Sure. I will do that.

@anandwana001 anandwana001 removed their assignment Jun 3, 2020
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Need some clarifications related to proto changes. Also, one of the reasons why the circle ci is failing is because of the changes in proto.

model/src/main/proto/profile.proto Outdated Show resolved Hide resolved
model/src/main/proto/profile.proto Outdated Show resolved Hide resolved
model/src/main/proto/profile.proto Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned anandwana001 and unassigned rt4914 Jun 3, 2020
model/src/main/proto/profile.proto Outdated Show resolved Hide resolved
model/src/main/proto/profile.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM. I have added one comment for Sarthak and Vinita.

model/src/main/proto/analytics.proto Show resolved Hide resolved
@rt4914 rt4914 assigned anandwana001 and unassigned rt4914 Jun 5, 2020
@anandwana001 anandwana001 removed their assignment Jun 5, 2020
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

LGTM

model/src/main/proto/analytics.proto Show resolved Hide resolved
@vinitamurthi
Copy link
Contributor

Looks good, there are a few unresolved comments though, please wait on that.

@rt4914
Copy link
Contributor

rt4914 commented Jun 5, 2020

@anandwana001 Resolved all my comments.

@rt4914 rt4914 removed their assignment Jun 5, 2020
@anandwana001
Copy link
Contributor Author

@anandwana001 Resolved all my comments.

Thank you

@rt4914
Copy link
Contributor

rt4914 commented Jun 5, 2020

Merging this on behalf of @anandwana001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a Protobuf linter to the project
4 participants