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

Adopt AEP-202: Fields #155

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

rambleraptor
Copy link
Member

Addresses #148

This adopts AEP-202: Fields

  • Created new folder for AEP 202
  • Changed references from AIP to AEP

Note: this still uses the google.api.field_info proto type.

@rambleraptor rambleraptor requested a review from a team as a code owner March 19, 2024 05:09
@rofrankel rofrankel self-requested a review March 22, 2024 19:21
aep/general/0202/aep.yaml Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
@rofrankel
Copy link
Collaborator

Added a few high-level comments - will do a more thorough read of the PR once those are resolved.

@rambleraptor
Copy link
Member Author

Added the proto file for aep.api.FieldInfo. PTAL!

@rambleraptor rambleraptor requested a review from rofrankel April 5, 2024 19:02
Copy link
Collaborator

@rofrankel rofrankel left a comment

Choose a reason for hiding this comment

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

Replied to comments, but have to send this to take myself out of the attention set.

@rambleraptor rambleraptor requested a review from rofrankel April 12, 2024 22:41
@mkistler
Copy link
Contributor

Should we generalize this guidance to cover format in OAS?

@rambleraptor
Copy link
Member Author

Should we generalize this guidance to cover format in OAS?

We should do that in a subsequent PR. I think they'll be a lot of conversation about that.

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0202/aep.md.j2 Show resolved Hide resolved
aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
aligning implementations between each consumer and each service without any
issue is infeasiable.

#### Why document value normalizations?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, what guidance does this refer to?

Copy link
Member Author

Choose a reason for hiding this comment

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

The above guidance should satisfy this rationale.

Copy link
Collaborator

@rofrankel rofrankel Jul 3, 2024

Choose a reason for hiding this comment

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

Which guidance, specifically? As in, which line of the file says to document value normalization? The word "normalization" doesn't appear before line 37.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't being precise enough about that. It was implicit. I've added some information in the beginning explaining how validators are useful for field normalization and why that process occurs.

Let me know if it isn't precise enough.

aep/general/0202/aep.md.j2 Outdated Show resolved Hide resolved
@rofrankel rofrankel self-requested a review June 7, 2024 18:41
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.

3 participants