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

refactor(schemas): house cleaning #9

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

Conversation

jeremyfiel
Copy link

couple issues in the existing json schema components that I cleaned up for JSON Schema tooling and validation

Also added the phone_number json schema that was missing. I'm not sure if the interpretation of the text is correct, specifically for the short_code. Please clarify if it's not correct.

related:

add `examples` to common components

update schema uri identifiers (`$id`)

relax constraints in decimal schema for reuse in money schema

Update markdown descriptions to align vocabulary with JSON Schema

[https://aep.dev/213] #134
@jeremyfiel jeremyfiel requested a review from a team as a code owner March 16, 2024 02:50
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

A couple places I'd like to check on with respect to json schema correctness. One question about the usage of the term field vs properties that I wanted to check in with @rofrankel about.

json_schema/decimal.yaml Show resolved Hide resolved
json_schema/decimal.yaml Outdated Show resolved Hide resolved
json_schema/money.yaml Outdated Show resolved Hide resolved
@@ -8,28 +8,28 @@ point representation in order to avoid precision errors.
A `Decimal` represents a decimal number in a form similar to scientific
notation.

It has two fields:
It has two properties:
Copy link
Member

Choose a reason for hiding this comment

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

I believe the reason these were described as "field" is because that's what protobuf calls them.

I think we had a discussion no what we would call the protocol-agnostic conflict, and landed on "fields" to match the existing AEP text. @rofrankel can correct me though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember the discussion, but at a first take that conclusion sounds good to my protobuf-biased self.

Aside from protobuf using one term and OAS using the other, is there an agnostic argument either way?

Copy link
Author

Choose a reason for hiding this comment

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

To be clear, the properties term is not OAS specific but rather JSON Schema derived. Here's a nice long summary from SO. IMO, because we discussing a schema definition, I would prefer the use of properties but I'm open to discussion on it.

https://stackoverflow.com/questions/295104/what-is-the-difference-between-a-field-and-a-property

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fair clarification...though from the perspective of trying to be agnostic to protobuf vs. OAS, OAS and JSON Schema are effectively the same entity (they're always and only used together with respect to protobuf being an alternative).

Copy link
Author

Choose a reason for hiding this comment

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

I'm ok either way.

jeremyfiel and others added 2 commits March 20, 2024 09:45
single quotes for mapping keys

Co-authored-by: Yusuke Tsutsumi <[email protected]>
update reference

Co-authored-by: Yusuke Tsutsumi <[email protected]>
Copy link
Contributor

@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.

Sorry for the delay in following up on this. It LGTM other than the properties vs. fields question. I don't necessarily have a strong opinion on this (yet), but if we switch from the protobuf terminology to the OAS terminology without an objective/agnostic reason for doing so, I'm not sure it's an improvement.

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