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 dictionary specs #49

Merged
merged 2 commits into from
Dec 28, 2021
Merged

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Dec 27, 2021

馃Л What and Why

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

Changes included:

  • Add dictionary endpoint specs
  • Update some ref that were not correct

馃И Test

CI :D

Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Looks good overall.
Could you please add a small test for each endpoint please ? ;)

content:
application/json:
schema:
$ref: './common/schemas/DictionaryEntriesResponse.yml#/dictionaryEntriesResponse'
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we say that we wanted to avoid refs like this when possible ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is not pointing to the properties, do you mean the ./ at the beginning?

Copy link
Contributor

Choose a reason for hiding this comment

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

No I mean, I thought we should simply add the properties directly in the file without any ref, as it's done here for example : https://github.com/algolia/api-clients-automation/blob/main/specs/search/paths/synonyms/clearAllSynonyms.yml#L19-L26

Copy link
Member Author

Choose a reason for hiding this comment

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

The same response is used in 3 endpoints, so IMO it made more sense to only have one definition. I can't remember if we had a discussion about this but I think we should reduce duplicate as mush as possible.

We could even have this generic response (updatedAt, taskId) as a common response for all of our clients. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay let's do this (same for deletedAt and createdAt maybe ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you list them all here?

I have a local branch that improve consistency between spec files, I can put it in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

  • taskID / updatedAt
  • taskID / deletedAt
  • taskID / createdAt
  • hits / nbHits ?
  • search / count / nbHits ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I'll try to make them generic and find a good name D:

Copy link
Contributor

Choose a reason for hiding this comment

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

馃憤

specs/search/paths/dictionaries/batchDictionaryEntries.yml Outdated Show resolved Hide resolved
@shortcuts
Copy link
Member Author

Could you please add a small test for each endpoint please ? ;)

I did not did it for this PR since it was already big, I will do a PR right after with some tests for these (and some others) endpoints. wdyt?

@damcou
Copy link
Contributor

damcou commented Dec 28, 2021

Could you please add a small test for each endpoint please ? ;)

I did not did it for this PR since it was already big, I will do a PR right after with some tests for these (and some others) endpoints. wdyt?

No problem :) . I just feel that I'm the only one to add tests until now ahah

@shortcuts
Copy link
Member Author

No problem :) . I just feel that I'm the only one to add tests until now ahah

True D:

@shortcuts shortcuts marked this pull request as ready for review December 28, 2021 10:41
@damcou damcou self-requested a review December 28, 2021 10:41
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

LGTM now :)

@shortcuts shortcuts merged commit 6587e94 into main Dec 28, 2021
@shortcuts shortcuts deleted the feat/APIC-194/dictionary-specs branch December 28, 2021 10:54
@shortcuts shortcuts self-assigned this Dec 28, 2021
shortcuts added a commit that referenced this pull request Apr 22, 2022
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