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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(source datasets): implementation #379

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

dmgauthier
Copy link
Contributor

Acceptance Criteria

  • JSDoc annotates each property added in the exported interfaces
  • The proposed changes are covered by unit tests
  • Commits containing breaking changes a properly identified as such
  • README.md is adjusted to reflect the proposed changes (if relevant)

@dmgauthier dmgauthier requested a review from a team as a code owner October 13, 2021 17:02
@dmgauthier dmgauthier force-pushed the feat-TRI-145-web-dataset-implementation branch from 5904213 to 40a30fd Compare October 13, 2021 17:26
Comment on lines +20 to +24
web = 'WEB',
/*
* Dataset for ecommerce search
*/
ecommerce = 'ecommerce',
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-bidding comment, because I assume this is as such in the backend but, I think it'd be nice to have consistency on the format of the DatasetType. Here we have an all-cap and an all-lowercase, that's a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is exactly like that in the backend. But I do agree with you and I dislike that 🙊 . But without those exact strings in the params we are going to hit 400s

@@ -2,3 +2,4 @@ export * from './Sources';
export * from './SourcesInterfaces';
export * from './SourcesFields/';
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-bidding either, but it'd be nice to uniformize here and make it goes as such:

Suggested change
export * from './SourcesFields/';
export * from './SourcesFields';

Just to be consistent.

@louis-bompart louis-bompart requested review from a team, toofff, mikedidomizio and sallainCoveo and removed request for olamothe and a team October 13, 2021 18:09
@dmgauthier dmgauthier force-pushed the feat-TRI-145-web-dataset-implementation branch 2 times, most recently from 5e36dcc to f1b92dc Compare October 14, 2021 14:07
@dmgauthier dmgauthier force-pushed the feat-TRI-145-web-dataset-implementation branch from f1b92dc to ed19efc Compare October 14, 2021 14:08
@dmgauthier dmgauthier merged commit 21d0a4b into master Oct 14, 2021
@dmgauthier dmgauthier deleted the feat-TRI-145-web-dataset-implementation branch October 14, 2021 14:08
@coveobot
Copy link

🎉 This PR is included in version 25.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants