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.

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

[Fix #679] Clear cache #782

Merged
merged 19 commits into from
Apr 8, 2021
Merged

[Fix #679] Clear cache #782

merged 19 commits into from
Apr 8, 2021

Conversation

Vitalina-Vakulchyk
Copy link
Contributor

@Vitalina-Vakulchyk Vitalina-Vakulchyk commented Apr 6, 2021 •
edited
Loading

Wrapped Elasticsearch::API::Indices::Actions#clear_cache with .clear_cache in Index class.

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Updated tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

dalthon
dalthon reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@dalthon dalthon left a comment

Choose a reason for hiding this comment

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

I believe that it would be very nice to have nicer default arguments

lib/chewy/index/actions.rb Outdated Show resolved Hide resolved
spec/chewy/index/actions_spec.rb Outdated Show resolved Hide resolved
spec/chewy/index/actions_spec.rb Outdated Show resolved Hide resolved
@Vitalina-Vakulchyk
Copy link
Contributor Author

@dalthon Thank you for useful comments. I have solved all of them.

dalthon
dalthon approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@dalthon dalthon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

🚀 Great.

Only suggested a few minor changes at spelling in specs.

Also remember to rename PR with Fix #679

spec/chewy/index/actions_spec.rb Outdated Show resolved Hide resolved
spec/chewy/index/actions_spec.rb Outdated Show resolved Hide resolved
bbatsov
bbatsov reviewed Apr 7, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@Vitalina-Vakulchyk Vitalina-Vakulchyk changed the title Clear cache [Fix #679] Clear cache Apr 7, 2021
@Vitalina-Vakulchyk Vitalina-Vakulchyk merged commit e4ed1eb into master Apr 8, 2021
@Vitalina-Vakulchyk Vitalina-Vakulchyk deleted the clear-cache branch April 8, 2021 10:27
cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023
@Vitalina-Vakulchyk
[Fix toptal#679] Clear cache (toptal#782)
e6b8394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rabotyaga rabotyaga rabotyaga approved these changes

@dalthon dalthon dalthon approved these changes

@bbatsov bbatsov Awaiting requested review from bbatsov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Vitalina-Vakulchyk @bbatsov @dalthon @rabotyaga