Skip to content

Initial pass at AEP-135 (Standard Methods: Delete)#94

Merged
rofrankel merged 4 commits intoaep-dev:mainfrom
mkistler:mdk/aep-135
Mar 14, 2024
Merged

Initial pass at AEP-135 (Standard Methods: Delete)#94
rofrankel merged 4 commits intoaep-dev:mainfrom
mkistler:mdk/aep-135

Conversation

@mkistler
Copy link
Copy Markdown
Contributor

This PR is a first cut at AEP-135, Standard Methods: Delete.

The effort highlighted a number of points we should discuss and settle on before the barn-raising. A short list:

  • Should we try to get AEP-8 (Style guide) in place before the barn-raising?
  • What to say about snake case field names convention
  • How to handle certain "proto-specific" terminology:
    • RPC
    • messages
    • return codes
  • HTTP Terminology
    • use "verb" instead of "method" for GET, PUT, POST, etc. Method is already overloaded.
  • We should have consistent order of oas and proto tabs (I like alpha order ;-))
  • Are there VSCode extensions we should recommend? Maybe
    • Better Jinja
    • Spectral
  • Can use use the simpler markdown reference syntax (drop trailing [])?

@mkistler mkistler requested a review from a team as a code owner February 11, 2024 21:55
Copy link
Copy Markdown
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.

Mostly looks good, but I noticed some copy/paste mistakes and also had one or two non-superficial questions.

I didn't properly review the embeddable OAS snippets because I don't have much OAS experience (maybe we can get Marsh to review those)...but once everything else is resolved I can take a look if helpful.

Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2
Comment thread aep/general/0135/aep.md.j2
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Copy link
Copy Markdown
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.

nice! left a few comments.

There's a few blocking conversations I'd love to talk through

  • eliminating deviation in returning the resource in the soft-delete pattern.
  • eliminating allow_missing.

we can keep discussing in-thread or maybe in our weekly?

Comment thread aep/general/0135/aep.md.j2
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/long_running_delete.oas.yaml
@mkistler
Copy link
Copy Markdown
Contributor Author

mkistler commented Mar 2, 2024

I think I have addressed all PR comments and marked them as resolved.

@rofrankel please review and approve or leave more feedback. Your approval is required as you originally requested changes.

Happy for anyone else that wants to review as well, but hoping to drive this to closure quickly.

Copy link
Copy Markdown
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.

Sorry for the latency on this.

Also, I may not have caught all the cases where "library" should change back to "google".

Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
Comment thread aep/general/0135/aep.md.j2 Outdated
@mkistler
Copy link
Copy Markdown
Contributor Author

What is the best path forward here? I will gladly restore all the original guidance for the Google AIP if that will unblock progress. Can we agree to take that as the initial cut and leave debates on the wisdom of Google's guidance to a future PR?

@mkistler
Copy link
Copy Markdown
Contributor Author

@rofrankel I've addressed most of your remaining comments. Tagged you to re-review.

Copy link
Copy Markdown
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.

Added a few non-trivial comments, otherwise LGTM.

@mkistler
Copy link
Copy Markdown
Contributor Author

@rofrankel I pushed back on the duplicated HTTP guidance and fixed all the rest.

Please re-review.

@mkistler mkistler requested a review from rofrankel March 12, 2024 12:31
Copy link
Copy Markdown
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.

LGTM on my side, thanks!

Comment thread aep/general/0135/aep.md.j2 Outdated
@mkistler mkistler enabled auto-merge (squash) March 13, 2024 20:28
@rofrankel rofrankel disabled auto-merge March 14, 2024 02:33
@rofrankel rofrankel merged commit 031fbe8 into aep-dev:main Mar 14, 2024
@mkistler mkistler deleted the mdk/aep-135 branch March 14, 2024 03:26
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