There’s a bunch of code quality issues on the main channel.

Going to start chipping away at these

so i tried to push changes to this discussion without success. I used:

pijul push danieleades@ssh.pijul.com:pijul/nest --to-channel main:837

doesn’t seem to have had the desired effect

EDIT: Should have been using pijul push danieleades@ssh.pijul.com:pijul/pijul --to-channel main:837, i guess i sent the changes to the wrong repo?

Cool! Thanks a lot. Before applying these, I’m a bit undecided about the following things, what do you think?

• I’m sure about touching the chardetng module, since it comes from another project. I’m afraid that if we modify it too much we might lose the ability to diff it with the upstream version (the diffs will be noisy).

• About the && vs nested if, I prefer nested ifs when the conditions “do something” (sort of like IO in Haskell), in my experience this is easier to debug.

• About manually implementing map, I actually prefer the manual implementation! I find it easier to read.

The rest looks good to me.

Since you ask, here’s what I think-

In all of these cases what you need to balance against your own preferences are:

1. the consistency of code style used by the community
2. the effectiveness of automated linting tools

Clippy is effectively a distillation of the community consensus of what good Rust code should like. There are enormous benefits to having a consistent, normative style across the community. Any contributor can pick up your repo and see a code style and idioms that they are familiar with. This is similar to the benefit provided by automated formatting.

Secondly, the codebase in its current form generates many hundreds of warnings/errors. This limits the effectiveness of clippy since the volume of issues makes it simply noise, and not useful feedback. It doesn’t help that there isn’t any CI preventing changes which introduce new warnings from being merged. Many clippy warnings relate to performance or correctness, or simply to new interfaces or syntax that can be leveraged to improve clarity.

So to ignore any particular warning it is not enough to decide that you prefer the style, you need to like it so much that it outweighs point 1. For me, that’s a very heavy cost. No doubt you weigh those differently to me, and that’s your prerogative.

Secondly, you need to selectively modify the linting to not produce warnings for things you don’t intend to change. That way, when there is a warning it’s a signal that it must actually be addressed. It should be a goal to get to 0 warnings, and stay at 0 warnings. Otherwise you have a “boy who cried wolf” problem and will miss meaningful warnings.

as for the chardetng module i’m not sure of the best approach. is there a way to remove/revert all changes to just that one directory?

having a module copied and pasted from another project is obviously a ‘code smell’. What is the reason for structuring it this way?

Clippy is effectively a distillation of the community consensus of what good Rust code should like. There are enormous benefits to having a consistent, normative style across the community. Any contributor can pick up your repo and see a code style and idioms that they are familiar with. This is similar to the benefit provided by automated formatting.

This is only partially true. Please take a look at the Clippy Repo’s Issue Tracker. The number of issues keeps growing, not shrinking, and this is a testament to how many hands Clippy needs v/s how many there are available for it.

Second, remember that there are more privileged hand owners than others, such as me. I have had the luxury of being able to give back some of my time directly to Clippy. But I am not representative of the broader community. Not everyone is a compiler nerd, and among the ones who are, not all of us have the time to spare and contribute.

These two things combined mean that the changes we see applied to Clippy are only a shadow of what the real community’s consensus is.

Last but not least: there is a reason why there are so many lint categories. The idea is to be able to use whichever lints make the more sense to the user.

Since unlike formatting, which is mostly about whitespace and case convention, the phrasing of the code itself conveys meaning. Style lints should always be optional, because at the end of the day, the compiler has no way of knowing what we meant – only we do. And not all code that behaves the same means the same.

These two things combined mean that the changes we see applied to Clippy are only a shadow of what the real community’s consensus is.

I don’t disagree with this, but as someone who manages the quality of a large codebase in my day job, i value consistency over even consensus. I accept not everyone sees it that way- that’s why the changes in this discussion are separated.

I have some changes to bring this up to date with main; should I push them?

Please!

I finally found the time to look into the Chardetng issue. Here’s what I did:

• I used pijul credit on libpijul/src/chardetng/mod.rs to figure out which patches modified it. One reason I was reluctant to modify that file too much is because I wanted to preserve pijul credit without having to dig very deep in that file’s history, since the UI to do that in Pijul is still weak. And any UI would have to deal with more info anyway.

• After finding the change IDs, I looked at the description of patches using pijul change, which had a mention of the chardetng crate.

• I looked at the PR I submitted with my changes, especially the dates.

That gave me the entire story: two years ago, I found an issue in chardetng, fixed it, submitted a PR on the original repo. At the time, the repo looked semi-abandonned: very few downloads on crates.io, last PR/issue months before that, and the maintainer took many days to ACK. At the time, I was trying to wrap up a release of Pijul, and the detection of character encoding was a minor fix introduced in that release, so I decided to copy my PR as a file in Pijul, and never found the time to get back to that.

I had completely forgotten about that story, so I’m glad the warning was there to remind me to delete the file.