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

Allow path field in update #86

Merged
merged 4 commits into from
Sep 16, 2024
Merged

Conversation

rambleraptor
Copy link
Member

Allow path field in Update to fix errors on aepc

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.

I'm ok if you want to follow up with a rule requiring path, but it'd be great to add that with this PR!

toumorokoshi added a commit to aep-dev/aepc that referenced this pull request Sep 6, 2024
Adding some tests to ensure:

- The generated output matches what is currently expected.
- Output is consistent with the AEP linter (this is disabled until aep-dev/api-linter#86 is merged).
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! There's still a fix needed, so if you can fix that before merging that would be great.

rule:
aep: 134
name: [core, '0134', request-path-required]
summary: Delete RPCs must have a `path` field in the request.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
summary: Delete RPCs must have a `path` field in the request.
summary: Update RPCs must have a `path` field in the request.

I think a few more places need "Delete" replaced with "Update".

Copy link
Member

Choose a reason for hiding this comment

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

A part of me wonders if we can make this rule or it's generation more generic somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially a way to do that? I can make a bug for it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably refactor the linter rule to look at specific method names (Read{singular}/Update{Singular}, etc), then require the field for those methods.

Maybe it'll be kind of hard to incorporate that into multiple different rule sets though.

@rambleraptor rambleraptor merged commit fcad800 into aep-dev:main Sep 16, 2024
1 check passed
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