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-122: Resource paths #63

Merged
merged 13 commits into from
Nov 17, 2023
Merged

Adopt AEP-122: Resource paths #63

merged 13 commits into from
Nov 17, 2023

Conversation

rofrankel
Copy link
Collaborator

@rofrankel rofrankel commented Oct 12, 2023

This is an adaptation of Roblox's AIP-122, which is an adaptation of Google's AIP-122.

This primary difference from Google's AIP-122 is the change from "name" to "path" for the key concept defined by the AEP.

Some notes on this PR:

  • This PR currently has protobuf-only guidance. If the content is generally acceptable to the working group, I'm happy to add the relevant OAS tabs.
  • The proposed new numbering scheme is ignored, because there are existing AEPs that don't use it. If and when we change the numbering scheme, a separate PR will be required anyway.
  • The PR uses apis.example.com/ (Roblox pattern) rather than ".exampleapis.com" (Google pattern). No strong feeling about this either way.

Addresses #55.

@rofrankel rofrankel requested a review from a team as a code owner October 12, 2023 02:00
@rofrankel rofrankel changed the title Adopt AEP-122: Resouce paths Adopt AEP-122: Resouce paths (#55) Oct 12, 2023
@rofrankel rofrankel changed the title Adopt AEP-122: Resouce paths (#55) Adopt AEP-122: Resouce paths Oct 12, 2023
@toumorokoshi toumorokoshi changed the title Adopt AEP-122: Resouce paths Adopt AEP-122: Resource paths Oct 12, 2023
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! a few comments to get the discussion going.

aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Show resolved Hide resolved
aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Outdated Show resolved Hide resolved
aep/general/0122/aep.md Show resolved Hide resolved
aep/general/0122/aep.md Show resolved Hide resolved
@toumorokoshi
Copy link
Member

Also, as discussed this would actually be AEP 1022 (or some AEP 1*** number)? since it's in the "spec" portion of the AEPs?

rofrankel and others added 6 commits October 23, 2023 14:24
This is an adaptation of Roblox's AIP-122, which is an adaptation of Google's AIP-122.

This primary difference from Google's AIP-122 is the change from "name" to "path" for the key concept defined by the AEP.

Some notes on this PR:

* This PR currently has protobuf-only guidance.  If the content is generally acceptable to the working group, I'm happy to add the relevant OAS tabs.
* The proposed new numbering scheme is ignored, because there are existing AEPs that don't use it.  If and when we change the numbering scheme, a separate PR will be required anyway.
* The PR uses apis.example.com/<api> (Roblox pattern) rather than "<api>.exampleapis.com" (Google pattern).  No strong feeling about this either way.
Co-authored-by: Yusuke Tsutsumi <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
@rofrankel
Copy link
Collaborator Author

Also, as discussed this would actually be AEP 1022 (or some AEP 1*** number)? since it's in the "spec" portion of the AEPs?

I think we converged on doing something with states instead, right?

@rofrankel
Copy link
Collaborator Author

@toumorokoshi any more comments on this?

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.

one minor change it would be great to address, but otherwise approved!

aep/general/0122/aep.yaml Outdated Show resolved Hide resolved
Co-authored-by: Yusuke Tsutsumi <[email protected]>
@rofrankel rofrankel merged commit 7fefa19 into main Nov 17, 2023
2 checks passed
@rofrankel rofrankel deleted the resource-paths branch November 17, 2023 20:48
toumorokoshi added a commit to toumorokoshi/aep.dev that referenced this pull request Jan 10, 2024
* Adopt AEP-122: Resouce paths

This is an adaptation of Roblox's AIP-122, which is an adaptation of Google's AIP-122.

This primary difference from Google's AIP-122 is the change from "name" to "path" for the key concept defined by the AEP.

Some notes on this PR:

* This PR currently has protobuf-only guidance.  If the content is generally acceptable to the working group, I'm happy to add the relevant OAS tabs.
* The proposed new numbering scheme is ignored, because there are existing AEPs that don't use it.  If and when we change the numbering scheme, a separate PR will be required anyway.
* The PR uses apis.example.com/<api> (Roblox pattern) rather than "<api>.exampleapis.com" (Google pattern).  No strong feeling about this either way.

* Update aep/general/0122/aep.md

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

* Update aep/general/0122/aep.md

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

* Address PR feedback from @toumorokoshi.

* Clarify wording around resource path uniqueness.

* Add information about how to annotate valid ID formats.

* Linkify `pattern`.

* Update aep/general/0122/aep.md

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

* Address comments from @toumorokoshi.

* Fix reference to JSON Schema keywords.

* Add a slug to AIP-122.

* Update aep/general/0122/aep.yaml

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

---------

Co-authored-by: Yusuke Tsutsumi <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
toumorokoshi added a commit to toumorokoshi/aep.dev that referenced this pull request Jan 24, 2024
* Adopt AEP-122: Resouce paths

This is an adaptation of Roblox's AIP-122, which is an adaptation of Google's AIP-122.

This primary difference from Google's AIP-122 is the change from "name" to "path" for the key concept defined by the AEP.

Some notes on this PR:

* This PR currently has protobuf-only guidance.  If the content is generally acceptable to the working group, I'm happy to add the relevant OAS tabs.
* The proposed new numbering scheme is ignored, because there are existing AEPs that don't use it.  If and when we change the numbering scheme, a separate PR will be required anyway.
* The PR uses apis.example.com/<api> (Roblox pattern) rather than "<api>.exampleapis.com" (Google pattern).  No strong feeling about this either way.

* Update aep/general/0122/aep.md

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

* Update aep/general/0122/aep.md

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

* Address PR feedback from @toumorokoshi.

* Clarify wording around resource path uniqueness.

* Add information about how to annotate valid ID formats.

* Linkify `pattern`.

* Update aep/general/0122/aep.md

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

* Address comments from @toumorokoshi.

* Fix reference to JSON Schema keywords.

* Add a slug to AIP-122.

* Update aep/general/0122/aep.yaml

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

---------

Co-authored-by: Yusuke Tsutsumi <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
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.

2 participants