-
Notifications
You must be signed in to change notification settings - Fork 14
IPA-110: Pagination (links array and includeCount not required) #622
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.
LGTM. Glad to see those
| const parameters = input.parameters; | ||
| if (!parameters) { | ||
| return; | ||
| } | ||
|
|
||
| const includeCountParam = parameters.find((p) => p.name === 'includeCount' && p.in === 'query'); | ||
|
|
||
| if (!includeCountParam) { | ||
| return; | ||
| } |
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.
are we allowed to use moderm js?
| const parameters = input.parameters; | |
| if (!parameters) { | |
| return; | |
| } | |
| const includeCountParam = parameters.find((p) => p.name === 'includeCount' && p.in === 'query'); | |
| if (!includeCountParam) { | |
| return; | |
| } | |
| const includeCountParam = input?.parameters?.find((p) => p.name === 'includeCount' && p.in === 'query'); | |
| if (!includeCountParam) { | |
| return; | |
| } |
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. We run above >18. ? operator was introduced 6 years ago to Node 14.
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.
Addressed 👍
| return; | ||
| } | ||
|
|
||
| if (includeCountParam.required === true) { |
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.
| if (includeCountParam.required === true) { | |
| if (!!includeCountParam.required) { |
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.
We use eslint for such style enforcement.
IMHO we can enable this rule https://eslint.org/docs/latest/rules/no-extra-boolean-cast
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.
Added no-extra-boolean-cast to eslint config with enforceForLogicalOperands: true
It does not allow if(!!includeCountParam.required), because JS already coerces the includeCountParam.required to boolean in if. I replaced it with if(includeCountParam.required)
| !isResourceCollectionIdentifier(resourcePath) || | ||
| (isResourceCollectionIdentifier(resourcePath) && isSingletonResource(getResourcePathItems(resourcePath, oas.paths))) |
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.
isn't this the same as
!isResourceCollectionIdentifier(resourcePath) || isSingletonResource(getResourcePathItems(resourcePath, oas.paths)!A || (A && B) = (!A || A) && (!A || B)
(!A || A) && (!A || B) = true && (!A || B)
true && (!A || B) = !A || B
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, it is :D It is widely used, so I will share another PR to fix this
Proposed changes
Jira ticket: CLOUDP-304954 and CLOUDP-304956
No violations found for
xgen-IPA-110-collections-request-includeCount-not-required11 violations found for
xgen-IPA-110-collections-response-define-links-array(similar to ones that does not have results array in the response)Checklist
Changes to Spectral
Further comments