Cleanup src/solver.rs

[xfbs]

This PR does some little refactoring of the resolve method.

• I rearranged the code so that it reduces nesting. I do that by rearranging conditionals so that the error case returns early and then paves the way for the (non-indented) success path.
• I give booleans a name (I tried, hope the names make sense) so that if statements "make more sense"
• I turn an "expect" into an error and remove an unwrap() by using a match statement.

[Eh2406]

Thanks for the improvements!

[mpizenberg]

Thanks for the PR! At first read I like most of it though I have a few remarks. We tend to be as specific as possible and favor reader over writer, so I'm not a super fan of unqualified import (for the log module).
Another nitpick, I'd prefer avoiding shadowing of variable (dependencies).

[xfbs]

Thanks for the PR!

Happy to help, awesome crate :) We use this in production btw!

At first read I like most of it though I have a few remarks. We tend to be as specific as possible and favor reader over writer, so I'm not a super fan of unqualified import (for the log module).

I definitely agree with you! I tend to always explicitly include anything I use, with only two exceptions: log::* and tracing::* (never both, just either one of them ofc), just because these crates are so useful everywhere and it's nice to be able to just plop a debug!() somewhere. But this is a personal preference, so I changed to explicitly importing them (I hope that is what you prefer).

Another nitpick, I'd prefer avoiding shadowing of variable (dependencies).

That is one feature of Rust I love :D but I get you, so I changed it, I hope my naming makes sense (I'm not too familiar with the codebase).

Feel free to also just take some part of my PR and throw out other parts! Style is always very subjective, I don't want to enforce my ideas, just sharing something that might be useful.

Cheers!

[Eh2406]

@mpizenberg are we good to merge now?

[mpizenberg]

If it's good for you, let me final check Sunday and I'll merge. If you don't here from me on Monday, consider it's good to merge :)

…

On Fri, Jun 9, 2023, 18:57 Jacob Finkelman ***@***.***> wrote: @mpizenberg <https://github.com/mpizenberg> are we good to merge now? — Reply to this email directly, view it on GitHub <#127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWFOCKEYUGZOBJ6ISYM7J3XKNIYHANCNFSM6AAAAAAXWEQZYI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mpizenberg]

Feels good to get rid of another unwrap and expect :)
Thanks again for the improvements.