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

Fix clippy warningis & errors #4

Merged
merged 3 commits into from
Jul 20, 2021
Merged

Conversation

yonipeleg33
Copy link

@yonipeleg33 yonipeleg33 commented Jul 10, 2021

A follow up to the issue I opened: #3
This PR is a step towards having a working CI/CD - only fixing clippy warnings / errors.

Edit: I fixed the warning below, @iximiuz please take a careful look there to make sure I maintained the logic there.

Note:

One error still remains which I'm currently not sure how to solve, and would like any help!

error: this loop never actually loops
   --> src/query/binary.rs:273:24
    |
273 |           let (lv, rv) = loop {
    |  ________________________^
274 | |             let (lv, rv) = match (self.left.peek(), self.right.peek()) {
275 | |                 (Some(InstantVector(lv)), Some(InstantVector(rv))) => (lv, rv),
276 | |                 (None, _) | (_, None) => return None,
...   |
301 | |             )));
302 | |         };
    | |_________^
    |
    = note: `#[deny(clippy::never_loop)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#never_loop

src/query/binary.rs Outdated Show resolved Hide resolved
@iximiuz
Copy link
Owner

iximiuz commented Jul 19, 2021

I definitely like this patch! Great job! I'll try to merge it tomorrow.

Copy link
Owner

@iximiuz iximiuz left a comment

Choose a reason for hiding this comment

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

Ok, I carefully went through all the changes and I'd be happy to merge all but two files. I'd ask you to revert changes to src/query/parser/expr.rs and src/query/binary.rs. For the first one, the function that got deleted is actually needed, I'm about to push a change where it's used. For the second one, as was discussed, there is probably a bug that needs to be fixed. I'm working on the test case right now.

@iximiuz
Copy link
Owner

iximiuz commented Jul 20, 2021

Oops, I remembered why I wrote a loop there... It's an old trick I learned from McConnell's Code Complete. It allows you to have some conditional logic w/o increasing the indentation level. I kind of dislike nested if's. Here is a short explanation from StackOverflow. Well, anyway, I added a test for this logic and got rid of the loop. PTAL #5

@yonipeleg33
Copy link
Author

Oops, I remembered why I wrote a loop there... It's an old trick I learned from McConnell's Code Complete. It allows you to have some conditional logic w/o increasing the indentation level. I kind of dislike nested if's. Here is a short explanation from StackOverflow. Well, anyway, I added a test for this logic and got rid of the loop. PTAL #5

So what should I do with the loop? I got confused lol
Should I merge from master and leave it as is?

@yonipeleg33
Copy link
Author

BTW @yonip23 is my other account, sorry for this 😄

@iximiuz
Copy link
Owner

iximiuz commented Jul 20, 2021

BTW @yonip23 is my other account, sorry for this 😄

Got it! 😉

I think you can just take the version of src/query/binary.rs from master and that would be it. Same for src/query/parser/expr.rs.

@yonipeleg33
Copy link
Author

BTW @yonip23 is my other account, sorry for this smile

Got it! wink

I think you can just take the version of src/query/binary.rs from master and that would be it. Same for src/query/parser/expr.rs.

I merged from your main branch and did as you said - please take a look to see that we're on the same page

@yonipeleg33 yonipeleg33 requested a review from iximiuz July 20, 2021 16:20
@iximiuz iximiuz merged commit e14e0ad into iximiuz:main Jul 20, 2021
@iximiuz
Copy link
Owner

iximiuz commented Jul 20, 2021

Yay! LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants