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: exclude etag from field_behavior lints #1009

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

noahdietz
Copy link
Collaborator

@noahdietz noahdietz commented Sep 6, 2022

The etag field should be excluded from the AIP-203 rules, because it is not supposed to be given any behavior annotations per AIP-154.

Fixes #977.

In a separate PR perhaps we should add a rule to enforce that etag should not have any field_behavior annotations at all.

@noahdietz noahdietz requested a review from a team as a code owner September 6, 2022 21:46
Copy link
Collaborator

@shwoodard shwoodard left a comment

Choose a reason for hiding this comment

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

I think I would rather have a const etagFieldName = "etag" and have all of the !excludeFields.Contains(f.GetName()) just be f.GetName() != etagFieldName rather than having a stringset with one value, imho.

rules/aip0203/aip0203.go Outdated Show resolved Hide resolved
@noahdietz
Copy link
Collaborator Author

I think I would rather have a const etagFieldName = "etag" and have all of the !excludeFields.Contains(f.GetName()) just be f.GetName() != etagFieldName rather than having a stringset with one value, imho.

I don't have another example of another standard field, but I can imagine there might be others, which is why I started with stringset. For example, I could see string name on the Resource being excluded from all of these. Just easier to expand later. Still want it changed?

@shwoodard
Copy link
Collaborator

I don't have another example of another standard field, but I can imagine there might be others, which is why I started with stringset. For example, I could see string name on the Resource being excluded from all of these. Just easier to expand later. Still want it changed?

Ack. Agreed. Can we change the variable name to standardFields?

@noahdietz
Copy link
Collaborator Author

Ack. Agreed. Can we change the variable name to standardFields?

Done!

@GregFurmanek
Copy link
Contributor

I am not sold on the standardFields name. It doesn't seem to describe the intended usage.

@shwoodard
Copy link
Collaborator

I am not sold on the standardFields name. It doesn't seem to describe the intended usage.

The variable name for a thing should say what it is and the behavior or how it is used should be demonstrated by the code, in this case, !....Contains(...)

@noahdietz
Copy link
Collaborator Author

I'm going to merge this. The variable name can be discussed here and updated in another PR if necessary.

@noahdietz noahdietz merged commit b521af0 into googleapis:main Sep 7, 2022
@noahdietz noahdietz deleted the etag-behavior branch September 7, 2022 00:04
@GregFurmanek
Copy link
Contributor

@shwoodard So we have named it standardFields? What are standard fields?
I think this use case is quite specific: Fields excluded from evaluation for specific rules.

"StandardFields" gives the reader no indication what this variable is going to be used for,
neither does !...Contains(...) for that matter.

I think making someone read the implementation to figure out what a variable is used for is
not vary kind way to write code.

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.

AIP-203: etag field should be excluded from field_behavior
4 participants