Skip to content

feat(specs): add dictionary specs #49

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 28, 2021
Merged

feat(specs): add dictionary specs #49

merged 2 commits into from
Dec 28, 2021

Conversation

@shortcuts
Copy link
Member

@shortcuts shortcuts commented Dec 27, 2021 •
edited
Loading

🧭 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
@shortcuts shortcuts requested a review from damcou December 27, 2021 16:56
damcou
damcou requested changes Dec 28, 2021
View reviewed changes
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 ? ;)
specs/search/paths/dictionaries/batchDictionaryEntries.yml
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.

👍
@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
damcou
damcou approved these changes Dec 28, 2021
View reviewed changes
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
@shortcuts
feat(specs): add dictionary specs (#49)
1b2f846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@shortcuts @damcou