-
Notifications
You must be signed in to change notification settings - Fork 0
disallow operationId #24
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Purpose: Make operationId optional by disabling its enforcing rule, updating example spec and documentation, and bumping package/dependency versions while updating supported Node.js versions.
- Disable operation-operationId rule (make operationId optional) and adjust test fixture.
- Remove corresponding documentation; bump package version and update dependencies / Node versions.
- Update CI and publish workflows to test/use Node 24.x.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/openapi.yml | Removed sample operationId to reflect new optional status. |
| ruleset.json | Disabled operation-operationId rule while keeping related validation rule. |
| package.json | Bumped version to 3.1.0; updated prettier config, spectral peer dep, and Node engine. |
| README.md | Removed guidance about mandatory operationId (left an empty section header). |
| .github/workflows/publish.yml | Switched runtime to Node 24.x. |
| .github/workflows/publish-beta.yml | Switched runtime to Node 24.x. |
| .github/workflows/ci.yml | Replaced Node 23.x matrix entry with 24.x. |
| .github/workflows/check-published-scheduled.yml | Switched runtime to Node 24.x. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ramaghanta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lftm
carlansley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node bump, but otherwise lgtm
package.json
Outdated
| }, | ||
| "engines": { | ||
| "node": ">=22.11" | ||
| "node": ">=22.15" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
22.18
|
I'm not sure we want to make this value optional. Its primary use is to help OAS visualizers, and other tooling, uniquely identify and reference operations. Making them optional sort of kills that use case. I'd vote for keeping them required since it's recommended by Spectral and all of our APIs already include them. That said, if we want to remove them I think we should just outright ban them. |
bobwieler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with @lukebrdn in that i don't think we want to make this optional. i've never found setting the operation id to be an issue until we started using typespec and with that gone i'd rather have this be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
following the comments from @lukebrdn and @bobwieler, we are now disallowing |
|
❌ PR review status - has 3 reviewers outstanding |
|
Beta Published - Install Command: |
|
we'll keep operationId to be explicit and avoid auto-generating stuff. so closing the PR. |
Fixes #23