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: various, mostly from cargo clippy -- --warn clippy::pedantic #77

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

JeromeSchmied
Copy link
Contributor

Code improvement

Description

Various code improvements, to make the code more readable, efficient and more Rusty

  • many functions from utils.rs were self impls -> removed them
  • in some cases, no mut was needed, instead of for ... ....iter().collect() was enough
  • applied numerous cargo clippy -- --warn clippy::pedantic fixes, eg.
    • unnecessary_not_if
    • let ... else
    • in some cases take Coord as value instead of reference, as it's more efficient
    • add ; to line-end if not returning
    • format!("{}", x) can be format!("{x}")
    • identical match branches
    • String::new() instead of "".to_string()
    • some fn-s take &str instead of String

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • My changes generate no new warnings, actually the opposite!
  • New and existing unit tests pass locally with my changes

Copy link
Owner

@thomas-mauran thomas-mauran left a comment

Choose a reason for hiding this comment

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

Nice fixes, we are having an overflow when eating a another piece thought we need to fix that

@JeromeSchmied
Copy link
Contributor Author

Nice fixes, we are having an overflow when eating a another piece thought we need to fix that

I'm sorry, didn't quite understand that.

@thomas-mauran
Copy link
Owner

Nice fixes, we are having an overflow when eating a another piece thought we need to fix that

I'm sorry, didn't quite understand that.

To reproduce the bug I found while running on your branch run a classic game, move a white pawn twice, same for black, then eat the black pawn with the white one and the game should quit with a rust panick

@JeromeSchmied
Copy link
Contributor Author

Actually it's interesting, because as far as I'm concerned, no code logic was changed before, so it shouldn't have happened, but luckily enough it seems to be fixed now, by commit a0a829b.

@thomas-mauran
Copy link
Owner

Actually it's interesting, because as far as I'm concerned, no code logic was changed before, so it shouldn't have happened, but luckily enough it seems to be fixed now, by commit a0a829b.

This is very weird yes, I'm gonna check in detail this commit tonight thank you for fixing it !

@thomas-mauran thomas-mauran added the refactor Refactor of the code label Aug 6, 2024
@thomas-mauran
Copy link
Owner

Perfect thanks again for yet another contribution @JeromeSchmied

@thomas-mauran thomas-mauran merged commit a875cf2 into thomas-mauran:main Aug 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants