Skip to content

feat(js): avoid wrapping bodyParams #77

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

Merged

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Jan 11, 2022

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-254

Changes included:

Avoid wrapping parameters in an object when only bodyParams are required.

🧪 Test

CI :D

@shortcuts shortcuts requested review from a team and damcou and removed request for a team January 11, 2022 13:41
@shortcuts shortcuts marked this pull request as ready for review January 11, 2022 13:41
@shortcuts shortcuts self-assigned this Jan 11, 2022
@shortcuts shortcuts force-pushed the feat/APIC-254/unwrap-bodyparams branch from 6e3061a to 5c256e5 Compare January 11, 2022 16:03
@shortcuts shortcuts requested a review from millotp January 11, 2022 16:43
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

This is beautiful !

@@ -3002,7 +2949,7 @@ export type SearchRulesProps = {
*/
indexName: string;
/**
* The searchRulesParams.
* The searchRulesParams parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all those comments really needed ? They juste repeat the name of the param, it's juste noise.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only when no definitions are provided in the specs, it's either that on remove the ESLint rules that requires a description and/or the one that requires a @param

Copy link
Member Author

Choose a reason for hiding this comment

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

actually if we keep it, it should be object not parameter

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I would remove the rule

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's go for no description required then, we need to make sure we don't forget to provide it in the specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the description for the parameter, it feels weird and I'd rather at least see it's an object

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with the comment after @param, but the ones in the Props is totally superfluous

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean in the type def? Definitely

Copy link
Member Author

Choose a reason for hiding this comment

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

seems better here 920dc83

@shortcuts shortcuts force-pushed the feat/APIC-254/unwrap-bodyparams branch from 7d3753a to daf57d7 Compare January 12, 2022 09:32
@shortcuts shortcuts force-pushed the feat/APIC-254/unwrap-bodyparams branch from daf57d7 to 920dc83 Compare January 12, 2022 10:02
@shortcuts shortcuts requested a review from millotp January 12, 2022 10:06
export type OperationIndexProps = {
/**
* The index in which to perform the request.
*/
indexName: string;
/**
* The operationIndexObject.
*/
operationIndexObject: OperationIndexObject;
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know where it's coming from but line 2787 the description is wrong for buildInOperations

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, should be fixed in 8f891f6

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I misread the comment and the type, It's all good now

@shortcuts shortcuts requested a review from millotp January 12, 2022 10:46
@shortcuts shortcuts merged commit bcf59a0 into feat/APIC-201/abtesting-specs Jan 12, 2022
@shortcuts shortcuts deleted the feat/APIC-254/unwrap-bodyparams branch January 12, 2022 10:51
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