feat(js): group parameters #46

shortcuts · 2021-12-16T12:40:56Z

🧭 What and Why

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

Changes included:

To improve our DX, we decided wrap parameters under an object which:

Provide type for each methods (better DX)
Allow us to introduce param changes without breaking the usage
Make usage consistent between methods

🧪 Test

CI :D

bodinsamuel · 2021-12-16T13:44:11Z

templates/javascript/api-single.mustache

-  * @param {{paramName}} {{^description}}The {{paramName}}{{/description}}{{#description}}{{{description}}}{{/description}}
-  {{/allParams}}
+  {{! We group parameters under an object if there are more than 5}}
+  {{^allParams.5}}


I would say this is create more inconsistencies than needed, we should stick to one coding style imo. Wdyt?

Like having everything under an object?

Hm.. this indeed creates inconsistencies but only for a few methods, while having objects everywhere would impact all the methods.

On a side note, this also creates inlined types (since it does not come from the specs), and would make the client much more verbose

@bodinsamuel should we postpone the merge of this PR until further discussion?

We can postpone if there is strong pushback on the team, otherwise if the team agreed with > 5 you can merge

(I personally stand with object all the time but it's just my opinion)

I'll ask in core-apiclients after xmas, this is not urgent anyway

damcou

LGTM

templates/javascript/api-single.mustache

specs/search/common/schemas/SearchParams.yml

playground/javascript/recommend.ts

bodinsamuel

LGTM 💯

clients/algoliasearch-client-javascript/client-analytics/src/analyticsApi.ts

playground/javascript/personalization.ts

tests/CTS/clients/search/getRule.json

…eOf/allOf

millotp

A couple comments, looking good !

millotp · 2022-01-04T08:48:25Z

tests/CTS/clients/search/browse.json

@@ -2,7 +2,7 @@
  {
    "method": "browse",
    "testName": "get browse results with minimal parameters",
-    "parameters": ["indexName"],
+    "parameters": [{ "indexName": "indexName" }],


Should the parameters be an object instead of an array if we always pass one object ?

Since parameters will diverge in every languages, yes.

I'd say we should have an object with language: parameters.

(We could also generate code snippets later from this file, since we have the usage of each methods for each languages 🤔 )

The people writing the CTS shouldn't care about the langage, I think the template should handle all the details (and the generator script can help by providing a parametersArray for example)

Good point, also makes the thing a bit magic, but it should be easier to handle it in the templates.

millotp · 2022-01-04T08:57:01Z

clients/algoliasearch-client-java-2/algoliasearch-core/com/algolia/search/SearchApi.java

@@ -2575,7 +2575,7 @@ public DeletedAtResponse deleteBy(
  /**
   * Delete all records matching the query. Remove all objects matching a filter (including geo
   * filters). This method enables you to delete one or more objects based on filters (numeric,
-   * facet, tag or geo queries). It doesn’t accept empty filters or a query.
+   * facet, tag or geo queries). It doesn&#39;t accept empty filters or a query.


Can you change {{&description}} to {{{description}}} (I'm not sure what the & does) in api-single.mustache to remove the ' ?

You can also use & to unescape a variable: {{& name}}. This may be useful when changing delimiters (see "Set Delimiter" below).

Not sure what is our use here 🤔

Yeah I don't know, we are not using delimiters yet.

Actually, adding the & removed the &#.. escaped chars, so I've added it to the Java template.

specs/search/common/schemas/SearchParams.yml

sbellone · 2022-01-05T08:41:46Z

clients/algoliasearch-client-javascript/recommend/src/recommendApi.ts

+  getRecommendations({
+    getRecommendations,
+  }: GetRecommendationsProps): Promise<GetRecommendationsResponse> {


Since GetRecommendations is already an object, we can pass it directly, I find it weird to write getRecommendations({ getRecommendations: {

Indeed, but it requires some investigation on how to do so via mustache files, since we can't do comparison.

We could have a special case for this client, but it's not the right way to handle it if it happens again

ok thanks 👌

(We should indeed not forget about this issue before releasing a stable client IMO, especially if it's the only case)

shortcuts requested a review from a team December 16, 2021 12:40

shortcuts self-assigned this Dec 16, 2021

shortcuts requested review from millotp and removed request for a team December 16, 2021 12:40

This was referenced Dec 16, 2021

feat(js): group parameters #45

Closed

feat(specs): add analytics specs and client. #36

Merged

bodinsamuel reviewed Dec 16, 2021

View reviewed changes

damcou previously approved these changes Dec 17, 2021

View reviewed changes

shortcuts added 4 commits December 29, 2021 12:05

feat(js): group parameters

975178d

remove unused var

c91cf05

remove eslint rule

84fe5bd

feat: wrap parameters for all methods

134a386

shortcuts dismissed damcou’s stale review via 134a386 December 29, 2021 11:58

shortcuts force-pushed the feat/APIC-227/javascript-group-parameters branch from 1e1749c to 134a386 Compare December 29, 2021 11:58

shortcuts added 3 commits January 3, 2022 10:21

Merge branch 'main' into feat/APIC-227/javascript-group-parameters

e941aa8

fix: cts, rename long params

404756b

fix: playgrounds

12001b6

shortcuts commented Jan 3, 2022

View reviewed changes

shortcuts requested review from damcou and bodinsamuel January 3, 2022 11:20

Merge branch 'main' into feat/APIC-227/javascript-group-parameters

3bd3e2c

bodinsamuel previously approved these changes Jan 3, 2022

View reviewed changes

clients/algoliasearch-client-javascript/client-analytics/src/analyticsApi.ts Show resolved Hide resolved

playground/javascript/personalization.ts Show resolved Hide resolved

tests/CTS/clients/search/getRule.json Show resolved Hide resolved

merge main

2061aae

shortcuts dismissed bodinsamuel’s stale review via 2061aae January 3, 2022 15:41

bodinsamuel previously approved these changes Jan 3, 2022

View reviewed changes

fix: merge main, remove weird quotes, deduplicate response, handle on…

2dd75fd

…eOf/allOf

shortcuts dismissed bodinsamuel’s stale review via 2dd75fd January 3, 2022 17:06

shortcuts requested a review from bodinsamuel January 3, 2022 17:07

bodinsamuel previously approved these changes Jan 3, 2022

View reviewed changes

millotp reviewed Jan 4, 2022

View reviewed changes

apply changes from suggestion

02fe53c

shortcuts dismissed bodinsamuel’s stale review via 02fe53c January 4, 2022 09:51

bodinsamuel approved these changes Jan 4, 2022

View reviewed changes

millotp approved these changes Jan 4, 2022

View reviewed changes

Merge branch 'main' into feat/APIC-227/javascript-group-parameters

9d0f83a

shortcuts merged commit 71470e3 into main Jan 4, 2022

shortcuts deleted the feat/APIC-227/javascript-group-parameters branch January 4, 2022 10:18

sbellone reviewed Jan 5, 2022

View reviewed changes

shortcuts added a commit that referenced this pull request Apr 22, 2022

feat(js): group parameters (#46)

a252605

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(js): group parameters #46

feat(js): group parameters #46

shortcuts commented Dec 16, 2021 •

edited

Loading

bodinsamuel Dec 16, 2021

shortcuts Dec 16, 2021

bodinsamuel Dec 16, 2021

shortcuts Dec 16, 2021 •

edited

Loading

shortcuts Dec 17, 2021

bodinsamuel Dec 17, 2021 •

edited

Loading

shortcuts Dec 17, 2021

damcou left a comment

bodinsamuel left a comment

millotp left a comment

millotp Jan 4, 2022

shortcuts Jan 4, 2022

millotp Jan 4, 2022

shortcuts Jan 4, 2022

millotp Jan 4, 2022

shortcuts Jan 4, 2022

millotp Jan 4, 2022

shortcuts Jan 4, 2022 •

edited

Loading

sbellone Jan 5, 2022

shortcuts Jan 5, 2022

sbellone Jan 5, 2022

shortcuts Jan 5, 2022

feat(js): group parameters #46

feat(js): group parameters #46

Conversation

shortcuts commented Dec 16, 2021 • edited Loading

🧭 What and Why

Changes included:

🧪 Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodinsamuel Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damcou left a comment

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts commented Dec 16, 2021 •

edited

Loading

shortcuts Dec 16, 2021 •

edited

Loading

bodinsamuel Dec 17, 2021 •

edited

Loading

shortcuts Jan 4, 2022 •

edited

Loading