fizyr / subst

Apache License 2.0
10 stars 4 forks source link

Some linting and minor upgrades #10

Closed nyurik closed 6 months ago

nyurik commented 10 months ago

P.S. this is the very first Rust project I saw that uses TABs... Your choice of course, but i was a bit surprised :)

de-vri-es commented 10 months ago

Looks good, thanks for the PR!

The only thing I don't think we should do is the pattern return Err(...)?;. I have actually done it in the past because I think it looks nice, but it is somewhat strange: the return never actually does anything, because the ? already returns.

I think in the past clippy even warned against doing this.

But the rest looks good! I don't fully agree with rustfmt everywhere, but I'll have a look if I can tweak it further after the PR :)

de-vri-es commented 10 months ago

I'm actually also not sure I like the #[must_use]. It can be nice if a function name sounds like it might have side effects when it doesn't, but for these functions I don't think that is the case.

For example, Vec::len() is also not marked #[must_use]. Could you elaborate a bit why you think these should be #[must_use]?

nyurik commented 8 months ago

@de-vri-es thanks for reviewing! Apologies it took a bit longer than I anticipated. I guess must_use is a bit of a hit or miss - i'm ok to add it to many cases, but i can see how it could get in a way, so removed. I am a bit of a stickler for formatting simply because it keeps diffs smaller - if everyone formats the same way, it is less likely for text to have big changes. The return Err()? vs return Err().into() is the hardest one - I personally prefer the least amount of text, especially for error handling as it tends to get in a way of reading. We even had a discussion on Clippy board about this specific case, but didn't come up with a "universal" accepted method. I removed it though, better to get the needed stuff in than to figure out the best way to paint the bike-shed :smile: .

de-vri-es commented 6 months ago

Rebased on main and changed the YAML match statement.

Thanks for the PR!