Skip to content

feat: add full clients architecture init #13

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 10 commits into from
Nov 25, 2021
Merged

Conversation

damcou
Copy link
Contributor

@damcou damcou commented Nov 23, 2021

Add full search client architecture init

Added:

  • Search Client
  • Personalization Client
  • Recommend Client
  • Query Suggestions Client
  • A/B Testing Client
  • Analytics Client
  • Insights Client

Won't be added for now (but added in JIRA)

  • Crawler Client
  • Monitoring Client
  • Usage Client
  • Light JS Client

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.

I did not check the endpoints in detail but looks good overall!

Copy link
Contributor

@DevinCodes DevinCodes left a comment

Choose a reason for hiding this comment

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

Quick question: would this generate a separate API client for each API, or would they still be part of the same package but in a different class?

@damcou
Copy link
Contributor Author

damcou commented Nov 23, 2021

Quick question: would this generate a separate API client for each API, or would they still be part of the same package but in a different class?

The goal is to have a class generated for each API Client (Search, Perso, Recommend, Analytics, etc...) in the same package. I created the first spec files to stick to the REST API list you mentioned during the last meeting (https://www.algolia.com/doc/api-reference/rest-api/)

Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

This seems better like this, however I think we should abstract a little bit more and remove the ref of client in the specs. Those are OpenApi not necessarily made for client.

I also think some hierarchy are inverted, schemas and responses can hardly be reused accros APIs.

I would go for somethings like this:

 > specs
   > search
     > common
       parameters.yml
       
     > paths
       > indexes
         getObject.yml
         
       > dictionnaries
     spec.yml
     
   > recommend
   > abtesting
   > perso
   > query_suggestion
   > common

wdyt?

@damcou
Copy link
Contributor Author

damcou commented Nov 24, 2021

This seems better like this, however I think we should abstract a little bit more and remove the ref of client in the specs. Those are OpenApi not necessarily made for client.

I also think some hierarchy are inverted, schemas and responses can hardly be reused accros APIs.

I would go for somethings like this:

 > specs
   > search
     > common
       parameters.yml
       
     > paths
       > indexes
         getObject.yml
         
       > dictionnaries
     spec.yml
     
   > recommend
   > abtesting
   > perso
   > query_suggestion
   > common

wdyt?

You're right, it looks better that way, I reorganized the folders

@damcou damcou requested a review from bodinsamuel November 24, 2021 15:05
@damcou damcou marked this pull request as ready for review November 24, 2021 15:30
@damcou damcou requested review from millotp and shortcuts November 24, 2021 15:31
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.

I think we have mixed commons here:

Those are common to all the API clients.

Other than that, LGTM :D

@damcou
Copy link
Contributor Author

damcou commented Nov 24, 2021

I think we have mixed commons here:

* [search/common/responses/common.yml](https://github.com/algolia/api-client-automation-experiment/blob/feat/APIC-181/full-specs-init/specs/search/common/responses/common.yml)

* [search/common/parameters.yml](https://github.com/algolia/api-client-automation-experiment/blob/feat/APIC-181/full-specs-init/specs/search/common/parameters.yml)

* [search/common/schemas/ErrorBase.yml](https://github.com/algolia/api-client-automation-experiment/blob/feat/APIC-181/full-specs-init/specs/search/common/schemas/ErrorBase.yml)

Those are common to all the API clients.

Other than that, LGTM :D

Ah shit, good catch :p

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.

Looking good ! I like the new structure, however there are issues with duplicate endpoints, I did not write them all but some files need to be merged like:
settings.yml:

get:
  ...

put:
  ...

@damcou damcou requested review from shortcuts and millotp November 25, 2021 10:45
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.

I think the content of specs/common/responses/common.yml should be in specs/common/parameters.yml

@@ -0,0 +1,8 @@
appId:
Copy link
Member

Choose a reason for hiding this comment

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

securitySchemes can be a file

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.

This seems like a good start, I guess we will fine tune/discover details once implementing the endpoints!

GG

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.

5 participants