Skip to content

feat(spec): add Predict client #129

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
merged 30 commits into from
Feb 11, 2022
Merged

feat(spec): add Predict client #129

merged 30 commits into from
Feb 11, 2022

Conversation

anghelalexandra
Copy link
Contributor

@anghelalexandra anghelalexandra commented Feb 10, 2022

🧭 What and Why

Predict JavaScript client for pilot phase, useful for creating demos and UI components.

🎟 JIRA Tickets: https://algolia.atlassian.net/browse/PRED-142, https://algolia.atlassian.net/browse/PRED-138

Changes included:

Create Predict client & spec, only the endpoint for reading the user profile for now. The API URL will be changed in the future once a more stable version of the API will be released.

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Mostly naming comments, thanks for contributing :D

content:
application/json:
schema:
title: params
Copy link
Member

Choose a reason for hiding this comment

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

methodNameParams is more descriptive and less likely to have duplicate conflict

Suggested change
title: params
title: fetchUserProfileParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the name params to have a cleaner DX, since the requestBody params are currently merged in a single object. So we can have:

const userProfile = await predictClient.fetchUserProfile({
  userId: 'user1',
  params: { modelsToRetrieve: [], typesToRetrieve: [] },
});

instead of

const userProfile = await predictClient.fetchUserProfile({
  userId: 'user1',
  fetchUserProfileParams: { modelsToRetrieve: [], typesToRetrieve: [] },
});

Copy link
Member

@shortcuts shortcuts Feb 10, 2022

Choose a reason for hiding this comment

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

We plan to flatten this nested object anyway so it's not a big deal if not done now, but I wanted to point it.

The JS client have separated clients, while other languages only have algoliasearch, which can cause duplicate if the generated Params type is used elsewhere, or if an other method uses the same naming logic.

@shortcuts
Copy link
Member

shortcuts commented Feb 10, 2022

Semantic PR title should be feat(spec): add Predict client

edit: sorry it's spec 😮‍💨

@anghelalexandra anghelalexandra changed the title Feat/predict pilot feat(specs): add Predict client Feb 10, 2022
@anghelalexandra anghelalexandra changed the title feat(specs): add Predict client feat(spec): add Predict client Feb 10, 2022
shortcuts
shortcuts previously approved these changes Feb 11, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Last non blocking comment

Looks good, GG :D

@anghelalexandra anghelalexandra merged commit 484e589 into main Feb 11, 2022
@anghelalexandra anghelalexandra deleted the feat/predict-pilot branch February 11, 2022 10:38
shortcuts added a commit that referenced this pull request Apr 22, 2022
* Fetch user profile endpoint

* Fix errors

* Add Javascript client

* Update configuration

* Add JavaScript API client

* Fetch user profile endpoint

* Fix errors

* Add Javascript client

* Add JavaScript API client

* Fix configuration

* Remove old files

* Remove long descriptions

* Clean up old files, regenerate client

* Disable enum validation

* Update request body to remove InlineObject name

* Remove old file

* Update requestBody name

* Add 400 error

* Add example

* Update example and config files

* Remove duplicate error spec, formatting

* Remove git push file

* Fix error base path

* Remove extra empty lines

* regenerate and fix host edge case

* Add response titles, regenerate client

* update clients/README.md

Co-authored-by: Clément Vannicatte <[email protected]>
Co-authored-by: Clément Vannicatte <[email protected]>
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