-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-328313: Quickfix for legacy custom methods #823
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
// legacy custom method - use end of path as custom method name | ||
if (!method) { | ||
method = nouns.pop(); | ||
resourceIdentifier = resourceIdentifier.slice(0, resourceIdentifier.lastIndexOf('/')); |
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.
Could you add a test case to cover this?
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.
Done, two fixes covered by the legacy custom method section of tests now
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.
[nit] Do we need test for empty operationIds/resourceIdentifier not in the shape we expected?
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
This PR implements a quickfix for legacy custom methods in operation ID generation to address issues with how custom method names are processed and singularized. The changes ensure that custom methods with camelCase names are handled correctly and that resource identifiers are properly updated when extracting legacy custom methods from paths.
- Fixes resource identifier handling for legacy custom methods by removing the last path segment
- Prevents incorrect singularization of custom method names that contain multiple words
- Updates test expectations to reflect the corrected behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
operationIdGeneration.js | Adds logic to update resource identifier for legacy methods and prevent singularization of camelCase custom methods |
operationIdGeneration.test.js | Updates test expectations and adds new test case for legacy custom method handling |
Comments suppressed due to low confidence (1):
tools/spectral/ipa/rulesets/functions/utils/operationIdGeneration.js:33
- The variable name 'camelCaseCustomMethod' is misleading. This boolean indicates whether the method has additional words after the verb, not specifically whether it's camelCase. Consider renaming to 'hasCustomMethodSuffix' or 'isMultiWordMethod'.
const camelCaseCustomMethod = method.length > verb.length;
|
||
let nouns = resourceIdentifier.split('/').filter((section) => section.length > 0 && !isPathParam(section)); | ||
|
||
// legacy custom method - use end of path as custom method name |
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.
[nit] can be separate method for handling legacy operations right?
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.
Yes, I could probably make a method that contains the extra logic and calls on this function. Will be something to consider when I'm implementing the verbosity override - might refactor for use in generating a shorter recommended opId
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.
LGTM
Debug for legacy custom methods case
Jira ticket: CLOUDP-328313