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

Readd warnings failing build in dev but exclude some unused warnings #1405

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

7h3kk1d
Copy link
Contributor

@7h3kk1d 7h3kk1d commented Oct 3, 2024

Disabled Warnings:
26: Unused variable
27: Unused Variable Strict
32: Unused value declaration
33: Unused open statement
34: Unused type declaration
35: Unused for-loop index
38: Unused extension
39: Unused rec flag
58: Missing cmx file This was already in use

Rationale:
I had previously disabled warnings in dev in #1378. The idea being that you shouldn't restrict the ability to build given warnings, just the ability to deploy. Unfortunately dune only announces warnings the first time a module is built so you could have long lived local issues for serious warnings, i.e. missing cases in pattern-matching.

Unused warnings are allowed because they show up regularly as part of writing code and are easy to fix after a build failure in CI by just removing the unused code.

- The idea is to make development easier by allowing code to be built with unused warnings
- We're still restrictive on some warnings because dune won't output all warnings on every build
@7h3kk1d 7h3kk1d requested a review from cyrus- October 3, 2024 15:03
@7h3kk1d 7h3kk1d marked this pull request as draft October 3, 2024 19:57
@7h3kk1d 7h3kk1d marked this pull request as ready for review October 3, 2024 21:05
@cyrus- cyrus- merged commit c1c0eba into dev Oct 4, 2024
2 checks passed
@cyrus- cyrus- deleted the sane-warnings branch October 4, 2024 14:37
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.

2 participants