Skip to content

Conversation

0xrusowsky
Copy link
Contributor

@0xrusowsky 0xrusowsky commented Aug 27, 2025

Motivation

ref:

Solution

  • introduce a new deny config flag
     /// Diagnostic level (minimum) at which the process should finish with a non-zero exit.
     pub enum DenyLevel {
         /// Always exit with zero code.
         #[default]
     	Never,
         /// Exit with a non-zero code if any warnings are found.
     	Warnings,
         /// Exit with a non-zero code if any notes or warnings are found.
     	Notes,
     }
  • modify the Linter trait so that fn lint() returns an error based on the deny flag and the emitted diagnostics.
     fn lint(&self, input: &[PathBuf], deny: DenyLevel, compiler: &mut Compiler) -> Result<()>

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes: NOT YET. However, introduces a user warning explaining that we decided to remove the --deny-warnings flag in favor of a new --deny flag that takes the DenyLevel.

TODO:

@0xrusowsky 0xrusowsky linked an issue Aug 28, 2025 that may be closed by this pull request
@0xrusowsky 0xrusowsky marked this pull request as ready for review August 28, 2025 05:54
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

makes sense, left couple of minor nits

@grandizzy
Copy link
Collaborator

@0xrusowsky should we also add tests for forge build with lint set to fail and make sure build pass / fail?

grandizzy
grandizzy previously approved these changes Aug 28, 2025
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm, pending others review / solar bump and update

zerosnacks
zerosnacks previously approved these changes Aug 28, 2025
@DaniPopes
Copy link
Member

we also have --deny-warnings, do we also integrate this?

@0xrusowsky
Copy link
Contributor Author

we also have --deny-warnings, do we also integrate this?

sure. should it fail with Level::Note too? or just Level:Warning?

@DaniPopes
Copy link
Member

warnings. could we also rename to keep it consistent? --deny warnings/notes

@0xrusowsky 0xrusowsky dismissed stale reviews from zerosnacks and grandizzy via 0fdcd45 August 28, 2025 09:24
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

lgtm, minor adjustments, pending solar release

Copy link
Contributor

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

smol nits otherwise lgtm

@0xrusowsky 0xrusowsky enabled auto-merge (squash) September 19, 2025 16:18
@0xrusowsky 0xrusowsky requested a review from onbjerg September 19, 2025 17:36
@0xrusowsky 0xrusowsky merged commit c7d9703 into master Sep 19, 2025
25 checks passed
@0xrusowsky 0xrusowsky deleted the rusowsky/lint-exit-code branch September 19, 2025 17:59
@github-project-automation github-project-automation bot moved this to Done in Foundry Sep 19, 2025
@grandizzy grandizzy added this to the v1.4.0 milestone Sep 22, 2025
@grandizzy grandizzy moved this from Done to Completed in Foundry Sep 22, 2025
@0xrusowsky 0xrusowsky mentioned this pull request Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

feat(forge-lint): return non-zero exit code when lint fails
5 participants