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

Add support to the pagination setting customization at the index level #1339

Merged
merged 17 commits into from
Oct 5, 2022

Conversation

vishalsodani
Copy link
Contributor

@vishalsodani vishalsodani commented Oct 1, 2022

Pull Request

What does this PR do?

Fixes #1298

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@bidoubiwa
Copy link
Contributor

Hello @vishalsodani,
Thank you very much for contributing to Meilisearch ❤️.
However, I am not available on the weekend, but I will be back on Monday 😊.

PS: This message was sent automatically!

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for your contribution 🔥

I see that you are still in draft.

I made a small change request. Also .code-samples.meilisearch.yaml should be updated! Example here

tests/pagination.test.ts Outdated Show resolved Hide resolved
@vishalsodani
Copy link
Contributor Author

Hey! Thanks for your contribution fire

I see that you are still in draft.

I made a small change request. Also .code-samples.meilisearch.yaml should be updated! Example here

ok. Thanks.

I pushed as draft since I was not sure about the conformity of the tests I had written to the tests as written in other files like stop_words.test.ts.

For example,
describe.each([{ permission: 'Master' }, { permission: 'Private' }])

What is the Private permission?

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Oct 3, 2022

Meilisearch has 1 master-key and 2 default keys. One being private that should have all rights except on the /keys route. To ensure that Meilisearch is working correctly we are doing some tests against Meilisearch in the SDK's.
So every test is ran using the master key and the private key!

@vishalsodani
Copy link
Contributor Author

Meilisearch has 1 master-key and 2 default keys. One being private that should have all rights except on the /keys route. To ensure that Meilisearch is working correctly we are doing some tests against Meilisearch in the SDK's. So every test is ran using the master key and the private key!

ok. Thanks.

@vishalsodani
Copy link
Contributor Author

Meilisearch has 1 master-key and 2 default keys. One being private that should have all rights except on the /keys route. To ensure that Meilisearch is working correctly we are doing some tests against Meilisearch in the SDK's. So every test is ran using the master key and the private key!

ok. But, locally I start a Meilisearch instance without any key. So, most of private tests are failing.

@vishalsodani vishalsodani marked this pull request as ready for review October 3, 2022 13:15
@bidoubiwa
Copy link
Contributor

As per the contributing guide you'll have to add the --master-key masterKey flag when launching your Meilisearch to make the tests run correctly

@bidoubiwa
Copy link
Contributor

Can you merge main into your branch and re-run the command to run Meilisearch? Meilisearch just released version v0.29

@bidoubiwa
Copy link
Contributor

Missing for the moment:

  • code-samples file update
  • Readme specification (at the bottom) update

@vishalsodani
Copy link
Contributor Author

Can you merge main into your branch and re-run the command to run Meilisearch? Meilisearch just released version v0.29

Done. Tests: 6 skipped, 1730 passed, 1736 total

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Could you update the tests in settings.test.ts and add the check of this new field as well?

README.md Outdated Show resolved Hide resolved
src/indexes.ts Outdated Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
src/indexes.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
@bidoubiwa
Copy link
Contributor

Could you run yarn style:fix to fix the styling issues?

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Sorry to come back as often with reviews, since there are a lot of changes everything does not pop immediately.

tests/pagination.test.ts Outdated Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
src/indexes.ts Outdated Show resolved Hide resolved
src/indexes.ts Outdated Show resolved Hide resolved
vishalsodani and others added 3 commits October 4, 2022 20:38
Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>
Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>
Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>
@bidoubiwa bidoubiwa added the enhancement New feature or request label Oct 4, 2022
src/indexes.ts Outdated Show resolved Hide resolved
@bidoubiwa
Copy link
Contributor

Also, add them bottom of your testing file you are missing some tests. See example in #1344

@vishalsodani
Copy link
Contributor Author

Sorry to come back as often with reviews, since there are a lot of changes everything does not pop immediately.

No issues.

@vishalsodani
Copy link
Contributor Author

Also, add them bottom of your testing file you are missing some tests. See example in #1344

Done.

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

A part from these changes everything looks good :) Could you fix your styling errors?

.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
.code-samples.meilisearch.yaml Outdated Show resolved Hide resolved
tests/pagination.test.ts Outdated Show resolved Hide resolved
@vishalsodani
Copy link
Contributor Author

A part from these changes everything looks good :) Could you fix your styling errors?

Done.

@bidoubiwa
Copy link
Contributor

bors try

meili-bors bot added a commit that referenced this pull request Oct 5, 2022
@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 5, 2022

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM 🔥🔥🔥🔥

@bidoubiwa
Copy link
Contributor

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 5, 2022

@meili-bors meili-bors bot merged commit 8ae2369 into meilisearch:main Oct 5, 2022
@bidoubiwa
Copy link
Contributor

Thanks again for contributing with Meilisearch ❤️
If you are participating in Hacktoberfest, and you would like to receive a small gift from Meilisearch too, don't forget to fill this form.

@vishalsodani
Copy link
Contributor Author

Thanks again for contributing with Meilisearch heart If you are participating in Hacktoberfest, and you would like to receive a small gift from Meilisearch too, don't forget to fill this form.

Thanks for guiding me through the process! 😍 👏 @bidoubiwa

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

Successfully merging this pull request may close these issues.

Add support to the pagination setting customization at the index level
2 participants