meilisearch / milli

Search engine library for Meilisearch ⚑️
MIT License
464 stars 81 forks source link

Fix clippy error to add clippy job on Ci #659

Closed unvalley closed 2 years ago

unvalley commented 2 years ago

Related PR

This PR is for #673

What does this PR do?

PR checklist

Please check if your PR fulfills the following requirements:

curquiza commented 2 years ago

@unvalley Sorry, since we merged https://github.com/meilisearch/milli/pull/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 commented 2 years ago

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

unvalley commented 2 years ago

@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?

curquiza commented 2 years ago

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 commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

curquiza commented 2 years ago

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

curquiza commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

unvalley commented 2 years ago

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 commented 2 years ago

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

unvalley commented 2 years ago

@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 commented 2 years ago

@unvalley thank you very much this is really appreciated πŸ™

unvalley commented 2 years ago

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

curquiza commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

unvalley commented 2 years ago

bors try

bors[bot] commented 2 years ago

:lock: Permission denied

Existing reviewers: click here to make unvalley a reviewer

unvalley commented 2 years ago

it might be bors ready.

unvalley commented 2 years ago

668 is may be overlapped changes with this #659 .

but reducing #659 diff is useful for reviewers imo😺

curquiza commented 2 years ago

Ok @unvalley thanks for letting me know! We will merge #668 first

unvalley commented 2 years ago

Thanks for running CI. I need to align the clippy outputs on CI and on my machine.

curquiza commented 2 years ago

@unvalley https://github.com/meilisearch/milli/pull/668 has been merged! Can you fix the merge conflicts? πŸ˜‡

ehiggs commented 2 years ago

Hey @unvalley sorry for the rebase I've handed you. If you need help with the rebase let me know.

unvalley commented 2 years ago

@ehiggs No worries, Thanks for fixing clippy errors!

unvalley commented 2 years ago

@curquiza Ok, I fixed conflicts. Could you run the CI again?

curquiza commented 2 years ago

bors try

unvalley commented 2 years ago

oops, it conflicts again.

bors[bot] commented 2 years ago

try

Merge conflict.

unvalley commented 2 years ago

@curquiza I fixed conflicts again πŸ™πŸ»

unvalley commented 2 years ago

okay, the errors relate to facets refactoring. I'll fix that.

curquiza commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

unvalley commented 2 years ago

@curquiza I fixed conflicts againπŸ˜† (sorry for trying bors many times!)

curquiza commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

unvalley commented 2 years ago

fixed rust fmtπŸ”₯

curquiza commented 2 years ago

Bors try

bors[bot] commented 2 years ago

try

Build failed:

unvalley commented 2 years ago

@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 commented 2 years ago

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

irevoire commented 2 years ago

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.

unvalley commented 2 years ago

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

curquiza commented 2 years ago

And thanks for being such reactive @unvalley! You are more than welcome to contributing again πŸ˜„

curquiza commented 2 years ago

@Kerollmops could you take a look at it? And if you approve the PR, please merge it πŸš€

bors[bot] commented 2 years ago

Build succeeded:

meili-bot commented 2 years ago

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.