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

Support user dictionary loading #491

Merged
merged 6 commits into from
Oct 10, 2023

Conversation

ellnix
Copy link
Collaborator

@ellnix ellnix commented Oct 4, 2023

Pull Request

Related issue

Fixes #488

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

I attempted to keep to the repo's style and make as few edits as possible.

@ellnix ellnix mentioned this pull request Oct 4, 2023
3 tasks
def dictionary
http_get("/indexes/#{@uid}/settings/dictionary")
end
alias get_dictionary dictionary
Copy link
Member

Choose a reason for hiding this comment

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

Same thing as the other PR, let's remove these aliases :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as the other PR, I am assuming you mean the getter only, and leaving the setter alias.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8680ee7) 99.60% compared to head (d3ca0de) 99.61%.

❗ Current head d3ca0de differs from pull request most recent head 78459aa. Consider uploading reports for the commit 78459aa to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #491   +/-   ##
=======================================
  Coverage   99.60%   99.61%           
=======================================
  Files           9        9           
  Lines         509      516    +7     
=======================================
+ Hits          507      514    +7     
  Misses          2        2           
Files Coverage Δ
lib/meilisearch/index.rb 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brunoocasali
Copy link
Member

Can you fix the conflicts here?

@ellnix ellnix force-pushed the support-user-dictionary-loading branch 2 times, most recently from 6b16bf0 to 8707a48 Compare October 7, 2023 12:39
@ellnix
Copy link
Collaborator Author

ellnix commented Oct 7, 2023

Can you fix the conflicts here?

Done. Rebased and force pushed, everything should be clean. Let me know if something else is required.

meili-bors bot added a commit that referenced this pull request Oct 9, 2023
493: Support text-separator customization r=brunoocasali a=ellnix

# Pull Request

## Related issue
Fixes #487 

## PR checklist
Please check if your PR fulfills the following requirements:
- [X] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [X] Have you read the contributing guidelines?
- [X] Have you made sure that the title is accurate and descriptive of the changes?

There are linter errors on both this pull request and the #491 since they add new settings, but they cannot reasonably be tackled while adding settings. A separate issue and PR would be more appropriate.

Co-authored-by: ellnix <103502144+ellnix@users.noreply.github.com>
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
brunoocasali
brunoocasali previously approved these changes Oct 9, 2023
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.

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 9, 2023

Build failed:

meili-bors bot added a commit that referenced this pull request Oct 9, 2023
491: Support user dictionary loading r=brunoocasali a=ellnix

# Pull Request

## Related issue
Fixes #488 

## PR checklist
Please check if your PR fulfills the following requirements:
- [X] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [X] Have you read the contributing guidelines?
- [X] Have you made sure that the title is accurate and descriptive of the changes?


I attempted to keep to the repo's style and make as few edits as possible.



Co-authored-by: ellnix <103502144+ellnix@users.noreply.github.com>
@ellnix
Copy link
Collaborator Author

ellnix commented Oct 9, 2023

@brunoocasali
The two linter errors are that

  • class Index is too long and
  • the settings spec block is too long.

I do not feel like these are problems that can be really addressed in this PR, perhaps some refactoring should be done?

@brunoocasali
Copy link
Member

I think we can just regenerate the rubocop todo file, and then address those issues in a new PR :)

@ellnix
Copy link
Collaborator Author

ellnix commented Oct 9, 2023

Noted, will do first thing tomorrow.

@ellnix ellnix force-pushed the support-user-dictionary-loading branch from 1e772d2 to 78459aa Compare October 10, 2023 10:52
@ellnix
Copy link
Collaborator Author

ellnix commented Oct 10, 2023

@brunoocasali all done & rebased, should be good to go with 1 click. Sorry for being slow on the catch up, I wasn't familiar with the rubocop auto gen workflow. I will hang around to rebase the other PRs when this one gets merged. Thanks for the patience.

@brunoocasali
Copy link
Member

I should thank you. In fact, you're doing an amazing job here!

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.

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 10, 2023

@ellnix ellnix closed this Oct 10, 2023
@meili-bors meili-bors bot merged commit c73ad5e into meilisearch:main Oct 10, 2023
6 checks passed
meili-bors bot added a commit that referenced this pull request Oct 10, 2023
493: Support text-separator customization r=brunoocasali a=ellnix

# Pull Request

## Related issue
Fixes #487 

## PR checklist
Please check if your PR fulfills the following requirements:
- [X] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [X] Have you read the contributing guidelines?
- [X] Have you made sure that the title is accurate and descriptive of the changes?

There are linter errors on both this pull request and the #491 since they add new settings, but they cannot reasonably be tackled while adding settings. A separate issue and PR would be more appropriate.

Co-authored-by: ellnix <103502144+ellnix@users.noreply.github.com>
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
meili-bors bot added a commit that referenced this pull request Oct 10, 2023
493: Support text-separator customization r=brunoocasali a=ellnix

# Pull Request

## Related issue
Fixes #487 

## PR checklist
Please check if your PR fulfills the following requirements:
- [X] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [X] Have you read the contributing guidelines?
- [X] Have you made sure that the title is accurate and descriptive of the changes?

There are linter errors on both this pull request and the #491 since they add new settings, but they cannot reasonably be tackled while adding settings. A separate issue and PR would be more appropriate.

Co-authored-by: ellnix <103502144+ellnix@users.noreply.github.com>
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
@brunoocasali brunoocasali added the enhancement New feature or request label Dec 11, 2023
@ellnix ellnix mentioned this pull request Jan 16, 2024
3 tasks
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.

[v1.4] Support user-dictionary loading
2 participants