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

Improve list of lints #79

Closed
wants to merge 3 commits into from
Closed

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Oct 3, 2023

No description provided.

Copy link
Owner

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I see how linting is good but I can also see how keeping this list up-to-date is non-trivial. I would prefer to write something like #![warn(all)] but I don't think it exists. I see a few options in this space:

  1. Manually lists as much lint as possible. The problem is maintenance.
  2. Only list lints with high benefit. That's kind of the current status.
  3. Have a script that keeps the list of lints up-to-date. Would be nice.

How did you come up with this list? Is it automatable?

lib/src/lib.rs Outdated Show resolved Hide resolved
@ia0 ia0 changed the title Add rustc and rustdoc lints Improve list of lints Oct 3, 2023
@ia0 ia0 added the enhancement label Oct 3, 2023
@ia0
Copy link
Owner

ia0 commented Oct 3, 2023

How did you come up with this list? The comment doesn't describe this. What is the criteria for helpful? Why is it not all allowed-by-default list?

@uklotzde
Copy link
Contributor Author

uklotzde commented Oct 3, 2023

How did you come up with this list? The comment doesn't describe this. What is the criteria for helpful? Why is it not all allowed-by-default list?

What answer do you expect? It was a manual process as documented and is of course opinionated. I prefer to enable as many ("helpful") checks as possible to enforce idiomatic code.

@uklotzde
Copy link
Contributor Author

uklotzde commented Oct 3, 2023

I have added many comments to document my rationale. If you think this is not sufficient please reword or add what you are missing. You may also close this PR and do it your way. This was just a proposal I consider beneficial.

@ia0
Copy link
Owner

ia0 commented Oct 3, 2023

I see. I was expecting a methodological approach possibly based on some results or a study (like something going over all crates on crates.io). In other words, something broke and you tried to fix it.

Yes, I'll have to take time to study the situation of lints in the community. I believe that having a long list of #![warn] is an indication that something is wrong with the language. If they are allowed-by-default there's probably a good reason (which may not apply to this crate which would thus justify having them as warn). Depending on the outcome, I might write a script to make sure this stays up-to-date. But I'll try to avoid as much as possible to have a long list of manual exceptions to the default behavior of the language, because this will bite in the future.

@uklotzde
Copy link
Contributor Author

uklotzde commented Oct 3, 2023

I even didn't start to add any Clippy lints which would suggest more idiomatic refactorings.

Personally, I would prefer less freedom and more strict defaults to improve maintainability. Unrealistic. Manifold opinions, expectations, use cases, and environments that general purpose programming languages have to balance will prevent that.

@ia0
Copy link
Owner

ia0 commented Oct 9, 2023

Thanks again for bringing this to my attention, your PR, and your time.

I'm closing this in favor of #84. I added a TODO to fix the unmaintenability and started a discussion here to eventually address it.

@ia0 ia0 closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants