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

Remove jsonindex #278

Merged
merged 6 commits into from
May 5, 2022
Merged

Remove jsonindex #278

merged 6 commits into from
May 5, 2022

Conversation

abhizer
Copy link
Contributor

@abhizer abhizer commented Apr 26, 2022

Pull Request

What does this PR do?

Fixes #187
#187

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?

Todo List:

  • Add fields updated_at, created_at and primary_key to Index
  • Make get_raw_index return the raw Value that's returned by Meilisearch
  • Update fetch_info to update Index instead of returning JsonIndex
  • Remove JsonIndex and update everything (like get_raw_index) that relies on it to use Index
  • Tests

P.S.: I think this requires a thorough review, as a lot of things have been changed.

Thank you so much for contributing to Meilisearch!

Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Hey, the PR looks really good!
Thanks a lot for your contribution 🎉

src/client.rs Outdated Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
@abhizer abhizer requested a review from irevoire April 29, 2022 14:39
irevoire
irevoire previously approved these changes May 2, 2022
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

@bidoubiwa or @brunoocasali, this PR looks good to me, I think we can merge when one of you have the time to review it 😁

@irevoire irevoire requested a review from bidoubiwa May 2, 2022 21:41
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Hi @abhizer thanks a lot for your contribution, it is really appreciated ❤️

I left some comments in your PR, just to make sure I understood everything!

src/client.rs Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
Commit Suggestion: Avoid creating a new Index in get_raw_index

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Commit Suggestion: Make test_fetch_info more readable

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
@brunoocasali brunoocasali self-requested a review May 4, 2022 18:20
@abhizer abhizer requested a review from irevoire May 4, 2022 18:31
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @abhizer ❤️

Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution once again!

@bidoubiwa bidoubiwa added the enhancement New feature or request label May 5, 2022
@bidoubiwa
Copy link
Contributor

Does this PR introduces breaking changes? @irevoire @abhizer

@irevoire
Copy link
Member

irevoire commented May 5, 2022

yes that's super breaking

@bidoubiwa bidoubiwa added breaking-change The related changes are breaking for the users and removed enhancement New feature or request labels May 5, 2022
@bidoubiwa
Copy link
Contributor

Should the code-samples be updated here?

@abhizer
Copy link
Contributor Author

abhizer commented May 5, 2022

Went through the code samples, didn't notice anything that'd need to be changed.

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.

bors merge

bors bot added a commit that referenced this pull request May 5, 2022
278: Remove jsonindex r=bidoubiwa a=abhizer

# Pull Request

## What does this PR do?
Fixes #187 
#187

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue? 
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

## Todo List: 
 - [x] Add fields updated_at, created_at and primary_key to Index
 - [x] Make get_raw_index return the raw Value that's returned by Meilisearch
 - [x] Update fetch_info to update Index instead of returning JsonIndex
 - [x] Remove JsonIndex and update everything (like get_raw_index) that relies on it to use Index
 - [x] Tests 

*P.S.: I think this requires a thorough review, as a lot of things have been changed.*

Thank you so much for contributing to Meilisearch!


Co-authored-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented May 5, 2022

Build failed:

@bidoubiwa
Copy link
Contributor

bors try

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

bors bot commented May 5, 2022

try

Build succeeded:

@bidoubiwa
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented May 5, 2022

Build succeeded:

@bors bors bot merged commit 9227ebb into meilisearch:main May 5, 2022
@bidoubiwa
Copy link
Contributor

Flacky tests see #280 and #281

@abhizer abhizer deleted the remove_jsonindex branch May 5, 2022 13:53
bors bot added a commit that referenced this pull request May 9, 2022
287: Update version for the next release (v0.17.0) r=curquiza a=brunoocasali

This version makes this package compatible with Meilisearch v0.27.0 🎉
Check out the changelog of [Meilisearch v0.27.0](https://github.com/meilisearch/meilisearch/releases/tag/v0.27.0) for more information on the changes.

## ⚠️ Breaking changes

* Remove jsonindex (#278) `@abhizer`

## 🚀 Enhancements

* Add cli-app example to meilisearch-rust (#279) `@salugi`
* Ensure nested field support (#273) `@bidoubiwa`
* Add new search parameters `highlightPreTag`, `highlightPostTag` and `cropMarker` (#274) `@bidoubiwa`
* Add User-Agent header to have analytics in every HTTP request (#254) `@brunoocasali`

Analytics is enabled by default in the server, but you can disable them by following [this guide](https://docs.meilisearch.com/learn/what_is_meilisearch/telemetry.html#how-to-disable-data-collection)
Also, of course, every analytics data we collect are **ANONYMOUS** [read the guide for more information](https://docs.meilisearch.com/learn/what_is_meilisearch/telemetry.html).



Thanks again to `@abhizer,` `@bidoubiwa,`  `@irevoire,` `@salugi` and a1! 🎉


Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add updatedAt and createdAt to Index
4 participants