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

Merges #201 with current master, adds support for development on Windows #233

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

koshell
Copy link

@koshell koshell commented May 5, 2024

Updated #201 to allow for easy merging.
While I was there I also fixed up the tests to allow them to run on windows.

All current tests pass.

I'm happy to fix up any other issues blocking this from merging.

Copy link
Collaborator

@antifuchs antifuchs left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for forward-porting #201! I fixed a thing on master and that broke mergeability here - it's a pretty straightforward rebase/merge though.

I have one comment on the lints, but that's all!

Comment on lines +71 to +79
# Due to #![deny(warnings)] you can't compile the crate without these lints disabled.
# These are simple to resolve lints, it's up to Andreas Fuchs if they should be resolved.
#
# -- koshell
[lints.clippy]
default_constructed_unit_structs = "allow"
single_char_pattern = "allow"
len_zero = "allow"
clone_on_copy = "allow"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I'd prefer to resolve warning-scale lints than to ignore them (unless resolving them is extremely noisy or leads to API clutter). Where do you see these trigger? I tried running clippy from stable rust 1.79.0, and it doesn't warn (with the clippy fixes from latest HEAD).

Copy link
Author

Choose a reason for hiding this comment

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

@antifuchs Looked over the master branch to confirm if it still produced clippy warnings.

#235 is a list of the warnings it spits out for me. I went through and tagged where in the code each warning originated instead of in the Cargo.toml to make it easier to review.

I left some suggestions in the pull request to resolve the lints.

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