Fix clippy error to add clippy job on Ci #659

unvalley · 2022-10-10T14:33:52Z

Related PR

This PR is for #673

What does this PR do?

~~add Run Clippy job to CI (rust.yml)~~
apply cargo clippy --fix command
fix some cargo clippy error manually (but warnings still remain on tests)

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?

665: Fixing piles of clippy errors. r=ManyTheFish a=ehiggs ## Related issue No issue fixed. Simply cleaning up some code for clippy on the march towards a clean build when #659 is merged. ## What does this PR do? Most of these are calling clone when the struct supports Copy. Many are using & and &mut on `self` when the function they are called from already has an immutable or mutable borrow so this isn't needed. I tried to stay away from actual changes or places where I'd have to name fresh variables. ## 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? Co-authored-by: Ewan Higgs <ewan.higgs@gmail.com>

curquiza · 2022-10-20T10:07:33Z

@unvalley
Sorry, since we merged #665 related to your work, you have a lot of git conflict. Can you fix them?
I would keep at least the addition of the CI in your PR, which is really useful for us! Thank you 🙏

unvalley · 2022-10-20T10:09:23Z

@curquiza No problem, I'll fix the conflicts!

unvalley · 2022-10-20T16:00:16Z

@curquiza I've fixed conflicts by accepting all current (main) change. The last commit passes cargo clippy (but as I described, there are still warnings).
Please take a look?

.github/workflows/rust.yml

curquiza · 2022-10-20T17:06:31Z

For me it's ok for the CI side @ManyTheFish (or some else in @meilisearch/core-team) I let you review the code base part before merging

curquiza · 2022-10-20T17:06:47Z

bors try

bors · 2022-10-20T17:07:25Z

try

Build failed:

Run Rustfmt

curquiza · 2022-10-20T17:11:25Z

@unvalley looks like the CIs are all failing, could you fix this before the core team review your PR?

curquiza · 2022-10-24T08:34:23Z

bors try

bors · 2022-10-24T08:35:01Z

try

Build failed:

Run Rustfmt

unvalley · 2022-10-24T11:54:42Z

I have to fix all warnings to pass the CI.

env:
  CARGO_TERM_COLOR: always
  RUSTFLAGS: "-D warnings"

The RUSTFLAGS -D warnings denies all warnings and this pr has warnings by clippy now.

curquiza · 2022-10-24T11:57:20Z

@unvalley, how big is it. Do you want to provide another PR before merging this one fixing all the conflicts?

unvalley · 2022-10-24T12:23:58Z

@curquiza
There are 24 clippy errors (by -D warnings).
So, that's not too big. I work to fix the errors in this PR and I use #allow(clippy::lint) in some places to merge this PR asap.

curquiza · 2022-10-24T14:04:12Z

@unvalley thank you very much this is really appreciated 🙏

unvalley · 2022-10-24T16:24:25Z

@curquiza Could you try bors? 🙏
I've checked that cargo clippy -- -D warnings finished successfully on my machine.

curquiza · 2022-10-24T17:37:25Z

bors try

bors · 2022-10-24T17:38:09Z

try

Build failed:

Run Rustfmt

unvalley · 2022-10-25T01:25:37Z

bors try

bors · 2022-10-25T01:25:39Z

🔒 Permission denied

Existing reviewers: click here to make unvalley a reviewer

unvalley · 2022-10-28T11:40:29Z

fixed rust fmt🔥

curquiza · 2022-10-28T11:49:03Z

Bors try

bors · 2022-10-28T11:52:11Z

try

Build failed:

Run Clippy

Remove clippy job Fix clippy error type_complexity Restore ambiguous change

unvalley · 2022-10-28T14:49:04Z

@curquiza
I finally removed the clippy job in this pr to receive a code review for the current code change.
I'll add the job in this pr.

curquiza · 2022-10-31T08:43:10Z

Ok @unvalley!
Can you remove the issue from the Development section (on the right) then?

irevoire

Hey @unvalley, to me we're almost finished, there is just two last things that I would like to check but overall I think we can merge

filter-parser/src/lib.rs

milli/src/search/facet/filter.rs

irevoire · 2022-10-31T16:08:58Z

Hey @unvalley, since this is quite a big PR and you had to rebase a lot of changes we wanted to thank you for your time and accept your contribution for the hacktoberfest even though we're probably not going to merge it before the end of the event.
That's why I gave you the hacktoberfest-accepted tag.
And if you would like to receive some swag from Meilisearch, please complete this form.

Co-authored-by: Tamo <irevoire@protonmail.ch>

unvalley · 2022-10-31T16:45:05Z

@irevoire Thanks for letting me know! I completed the form. I'd like to contribute more.

curquiza · 2022-10-31T17:26:58Z

And thanks for being such reactive @unvalley! You are more than welcome to contributing again 😄

irevoire

Good to me, since the PR is quite large I'll let someone else from the @meilisearch/core-team double-review + merge

curquiza · 2022-11-03T12:59:00Z

@Kerollmops could you take a look at it? And if you approve the PR, please merge it 🚀

Kerollmops

There are a lot of changes but it looks good to me too!
Thank you very much ❤️

bors merge

bors · 2022-11-03T15:54:43Z

Build succeeded:

meili-bot · 2022-11-03T15:54:50Z

This message is sent automatically

Thank you for contributing to Meilisearch. If you are participating in Hacktoberfest, and you would like to receive some gift from Meilisearch too, please complete this form.

ehiggs mentioned this pull request Oct 13, 2022

Fixing piles of clippy errors. #665

Merged

3 tasks

unvalley marked this pull request as ready for review October 15, 2022 01:28

unvalley changed the title ~~Add clippy job to CI~~ Add clippy job to CI and fix clippy error Oct 15, 2022

curquiza requested a review from ManyTheFish October 17, 2022 08:59

curquiza self-requested a review October 20, 2022 10:07

curquiza reviewed Oct 20, 2022

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

curquiza added the no breaking The related changes are not breaking (DB nor API) label Oct 20, 2022

bors bot added a commit that referenced this pull request Oct 20, 2022

Try #659:

68cc94f

bors bot added a commit that referenced this pull request Oct 24, 2022

Try #659:

3bb916f

bors bot added a commit that referenced this pull request Oct 24, 2022

Try #659:

a52fb15

ehiggs mentioned this pull request Oct 24, 2022

Fix many Clippy errors part 2 #668

Merged

3 tasks

Fix rust fmt

f3c0b05

bors bot added a commit that referenced this pull request Oct 28, 2022

Try #659:

494f520

unvalley changed the title ~~Add clippy job to CI and fix clippy error~~ Fix clippy error to add clippy job on Ci Oct 28, 2022

unvalley added 3 commits October 28, 2022 22:33

fix clippy error and remove clippy job from ci

a1d7ed1

Remove clippy job Fix clippy error type_complexity Restore ambiguous change

Fix clippy error

d53a80b

Execute cargo fmt

d55f0e2

irevoire suggested changes Oct 31, 2022

View reviewed changes

filter-parser/src/lib.rs Outdated Show resolved Hide resolved

milli/src/search/facet/filter.rs Outdated Show resolved Hide resolved

irevoire added the hacktoberfest-accepted label Oct 31, 2022

Update filter-parser/src/lib.rs

0d43ddb

Co-authored-by: Tamo <irevoire@protonmail.ch>

curquiza mentioned this pull request Oct 31, 2022

Add clippy job #673

Merged

3 tasks

unvalley requested review from irevoire and removed request for ManyTheFish November 1, 2022 12:53

refactor: match for filterCondition

13175f2

unvalley requested review from Kerollmops and removed request for irevoire November 3, 2022 08:36

irevoire approved these changes Nov 3, 2022

View reviewed changes

Kerollmops approved these changes Nov 3, 2022

View reviewed changes

bors bot merged commit 6add470 into meilisearch:main Nov 3, 2022

unvalley deleted the add-clippy-to-ci branch November 3, 2022 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix clippy error to add clippy job on Ci #659

Fix clippy error to add clippy job on Ci #659

unvalley commented Oct 10, 2022 •

edited

Loading

curquiza commented Oct 20, 2022

unvalley commented Oct 20, 2022

unvalley commented Oct 20, 2022 •

edited

Loading

curquiza commented Oct 20, 2022

curquiza commented Oct 20, 2022

bors bot commented Oct 20, 2022

curquiza commented Oct 20, 2022

curquiza commented Oct 24, 2022

bors bot commented Oct 24, 2022

unvalley commented Oct 24, 2022

curquiza commented Oct 24, 2022

unvalley commented Oct 24, 2022 •

edited

Loading

curquiza commented Oct 24, 2022

unvalley commented Oct 24, 2022 •

edited

Loading

curquiza commented Oct 24, 2022

bors bot commented Oct 24, 2022

unvalley commented Oct 25, 2022

bors bot commented Oct 25, 2022

unvalley commented Oct 28, 2022

curquiza commented Oct 28, 2022

bors bot commented Oct 28, 2022

unvalley commented Oct 28, 2022

curquiza commented Oct 31, 2022

irevoire left a comment

irevoire commented Oct 31, 2022

unvalley commented Oct 31, 2022

curquiza commented Oct 31, 2022

irevoire left a comment •

edited

Loading

curquiza commented Nov 3, 2022 •

edited

Loading

Kerollmops left a comment

bors bot commented Nov 3, 2022

meili-bot commented Nov 3, 2022

Fix clippy error to add clippy job on Ci #659

Fix clippy error to add clippy job on Ci #659

Conversation

unvalley commented Oct 10, 2022 • edited Loading

Related PR

What does this PR do?

PR checklist

curquiza commented Oct 20, 2022

unvalley commented Oct 20, 2022

unvalley commented Oct 20, 2022 • edited Loading

curquiza commented Oct 20, 2022

curquiza commented Oct 20, 2022

bors bot commented Oct 20, 2022

try

curquiza commented Oct 20, 2022

curquiza commented Oct 24, 2022

bors bot commented Oct 24, 2022

try

unvalley commented Oct 24, 2022

curquiza commented Oct 24, 2022

unvalley commented Oct 24, 2022 • edited Loading

curquiza commented Oct 24, 2022

unvalley commented Oct 24, 2022 • edited Loading

curquiza commented Oct 24, 2022

bors bot commented Oct 24, 2022

try

unvalley commented Oct 25, 2022

bors bot commented Oct 25, 2022

unvalley commented Oct 28, 2022

curquiza commented Oct 28, 2022

bors bot commented Oct 28, 2022

try

unvalley commented Oct 28, 2022

curquiza commented Oct 31, 2022

irevoire left a comment

Choose a reason for hiding this comment

irevoire commented Oct 31, 2022

unvalley commented Oct 31, 2022

curquiza commented Oct 31, 2022

irevoire left a comment • edited Loading

Choose a reason for hiding this comment

curquiza commented Nov 3, 2022 • edited Loading

Kerollmops left a comment

Choose a reason for hiding this comment

bors bot commented Nov 3, 2022

meili-bot commented Nov 3, 2022

unvalley commented Oct 10, 2022 •

edited

Loading

unvalley commented Oct 20, 2022 •

edited

Loading

unvalley commented Oct 24, 2022 •

edited

Loading

unvalley commented Oct 24, 2022 •

edited

Loading

irevoire left a comment •

edited

Loading

curquiza commented Nov 3, 2022 •

edited

Loading