-
Notifications
You must be signed in to change notification settings - Fork 21
feat(specs): enrich response for list composition endpoint #4819
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
✔️ Code generated!
📊 Benchmark resultsBenchmarks performed on the method using a mock server, the results might not reflect the real-world performance.
|
7d7aff0
to
3d9fb97
Compare
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.
✅
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.
thanks ! Is this ready to be merged ?
@millotp Sorry I just rebased as Github was complaining about not being up to date with |
no worries, this is required because the CI will push a generated commit once this is merged |
Ok I see. I checked if I have to generate other stuff please do not hesitate if some stuff are missing in this PR 🙏 |
I think this is all good 😄 |
…enerated) [skip ci] Co-authored-by: Clara Muller <[email protected]>
algolia/api-clients-automation#4819 Co-authored-by: algolia-bot <[email protected]> Co-authored-by: Clara Muller <[email protected]>
page: | ||
type: integer | ||
description: Current page. | ||
example: 0 | ||
hitsPerPage: | ||
type: integer | ||
description: Number of items per page. | ||
example: 20 | ||
nbHits: | ||
type: integer | ||
description: Number of items. | ||
example: 200 |
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'm a bit late to the party, but shouldn't those be required as they are always returned @ClaraMuller ?
cc: @millotp
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.
(the Typescript types generated by the CI are optional which made me realize)
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.
What would happen if the field are not in the response (for whatever reason, for example due to a bug).
I would have though that by default no fields is required in the response to prevent having issue in the client side if something is going wrong in the backend.
But I don't have a strong opinion, I will update if we think this is important.
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.
What would happen if the field are not in the response (for whatever reason, for example due to a bug).
There would be an error because we wouldn't handle it.
But if there's a bug in the backend, that's fine imho.
The specs should represent the expected shape of data when there is no bug.
Otherwise it's unmanageable on the frontend to handle all cases when all data is optional.
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 will make them required then.
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/CMP-424
Changes included:
List composition endpoint response is enriched with response fields added since:
🧪 Test
yarn cli build specs all
yarn cli build clients javascript