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

Proj0039: Treat all warnings as errors is considered a bad practice #109

Merged
merged 15 commits into from
Mar 23, 2025

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Mar 9, 2025

I'm aware that this rule is opinionated, and I'm intending to release this with a severity info, instead of a warning. I'm also aware that this is by far the longest rule description, and is written more as a blog. Tips on how to improve that are (also) welcomed.

@Corniel Corniel added the documentation Improvements or additions to documentation label Mar 9, 2025
@Corniel Corniel self-assigned this Mar 9, 2025
Copy link
Contributor

@Gemberkoekje Gemberkoekje left a comment

Choose a reason for hiding this comment

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

I think it makes sense, although as you mention, some people will disagree.

@Corniel
Copy link
Contributor Author

Corniel commented Mar 9, 2025

I think it makes sense, although as you mention, some people will disagree.

Are there arguments you disagree with? Sentences you would phrase differently? May be swap the order of the arguments?

Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
@LauraXiulan
Copy link
Contributor

I think I would end with developer unwillingness, first mention the technical reasons then the 'emotional' ones

Corniel and others added 5 commits March 9, 2025 10:46
Co-authored-by: Gemberkoekje <Arie_Hofland@hotmail.com>
Co-authored-by: Gemberkoekje <Arie_Hofland@hotmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
@CptWesley
Copy link
Member

Perhaps it's worth mentioning it is also possible to treat warnings as errors only during the CI pipeline, so that it doesn't block building locally

Corniel and others added 6 commits March 9, 2025 10:49
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
Co-authored-by: Laura Kramer <laurakramer91@gmail.com>
@Corniel
Copy link
Contributor Author

Corniel commented Mar 9, 2025

Perhaps it's worth mentioning it is also possible to treat warnings as errors only during the CI pipeline, so that it doesn't block building locally

Sure, but that does not solve the main issue: it is rigid and contra-productive to have this enabled. Zero warnings should not be the main objective, having good code quality is. You could also say that this is of the category: If a measurement becomes a target, it looses is value as measurement.

So having the (only) enabled on the CI will not safe the day.

@Corniel
Copy link
Contributor Author

Corniel commented Mar 9, 2025

I think I would end with developer unwillingness, first mention the technical reasons then the 'emotional' ones

So in which numerical order?

And why? Do you think that are the stronger arguments? Or do you think you should end with the strongest arguments? I think the first one is the strongest argument: you come op with some rigid setting to overcome a social issue, and that will not work.

@LauraXiulan
Copy link
Contributor

Because it might put people off reading first that they are supposedly not willing to solve warnings, instead of first telling them why it might be a bad idea

@LauraXiulan
Copy link
Contributor

The rest of the order is fine, maybe put the IDE last (because it is the last hurdle in the process)

@Corniel Corniel merged commit 23c419f into main Mar 23, 2025
2 checks passed
@Corniel Corniel deleted the warnings-as-errors branch March 23, 2025 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants