Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(specs): add personalizaton spec and client #27

Merged
merged 10 commits into from
Dec 6, 2021

Conversation

shortcuts
Copy link
Member

馃Л What and Why

馃師 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-203

Changes included:

  • Add personalization spec and client
  • Update contribution guide with new host logic related to the client

@shortcuts shortcuts self-assigned this Dec 2, 2021
@shortcuts shortcuts requested review from millotp and damcou and removed request for millotp December 2, 2021 15:21
@shortcuts shortcuts marked this pull request as ready for review December 2, 2021 15:21
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.

That's impressive ! I need to catch up the tests 馃槢

doc/contribution_addNewClient.md Outdated Show resolved Hide resolved
openapitools.json Show resolved Hide resolved
"npmName": "@algolia/client-personalization",
"npmVersion": "5.0.0",

"isPersonalizationHost": false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only personalization uses different hosts ?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now yes, I guess we should iterate on that if we encounter that again

playground/javascript/tsconfig.json Outdated Show resolved Hide resolved
specs/common/parameters.yml Outdated Show resolved Hide resolved
templates/javascript/api-single.mustache Outdated Show resolved Hide resolved
templates/javascript/api-single.mustache Outdated Show resolved Hide resolved
dist
build
.git_push.sh
.openapi-generator-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

.openapi-generator-ignore is used in other languages, and can be used here too, to put git_push.sh and openapi-generator inside for example, so that they won't get generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean we can add a .openapi-generator-ignore to the template folder? 馃

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately no, in must be at the root of the generated folder, where it is already, and never destroyed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not try, maybe it can be copied with a script instead

@millotp
Copy link
Collaborator

millotp commented Dec 2, 2021

Don't forget to add it to the CI also

millotp
millotp previously approved these changes Dec 2, 2021
@@ -8,6 +8,14 @@ IndexName:
type: string
example: 'myIndexName'

UserToken:
Copy link
Collaborator

@millotp millotp Dec 2, 2021

Choose a reason for hiding this comment

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

It's weird to have a common parameter depends on a personalization param, does this belong in personalization ? Or userToken belong in common ?

Copy link
Member Author

Choose a reason for hiding this comment

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

True! I think it's used later for API keys but we're not there yet, I've moved it to the personalization folder

{{/isPersonalizationHost}}

{{#isPersonalizationHost}}
public getDefaultHosts(region: string = 'us'): Host[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have a list of region somewhere ? this could be an union (unless infra likes to add a new region everyday)

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 don't think so, at least did not found one. We will also need it for A/B testing, so it would be could to list them all indeed

millotp
millotp previously approved these changes Dec 6, 2021
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.

Cool ! I don't know if this is in this PR but the file client-search/model/listIndicesObject.ts is commited but not generated, could you remove it please if this is the right scope ?

@shortcuts
Copy link
Member Author

could you remove it please if this is the right scope ?

Good catch! Not related to this PR but not a big deal, done in 3c50767

@shortcuts shortcuts merged commit c1ac7be into main Dec 6, 2021
@shortcuts shortcuts deleted the feat/personalization-specs branch December 6, 2021 09:54
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.

None yet

2 participants