Skip to content

Add --allow-warnings flag to treat certain errors as warnings#481

Merged
jfecher merged 6 commits intomasterfrom
jf/unused-warning2
Nov 17, 2022
Merged

Add --allow-warnings flag to treat certain errors as warnings#481
jfecher merged 6 commits intomasterfrom
jf/unused-warning2

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented Nov 16, 2022

Related issue(s)

Relates to "Change unused variables to warnings" #263.

Description

This PR has the same effect as #263 but puts this behavior behind a compile flag --allow-warnings. The "allow-warnings" flag was chosen instead of a more specific flag relating to unused variables because of the implementation in the Diagnostics reporter which simply changes all Warnings to errors if the flag is not set.

Thanks to @vezenovm for adding the flag to #263. This is created as a new PR on a new branch since I opted to start over and cherry-pick + manually apply changes instead of fixing 12k+ lines of conflicts with master.

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional Information

This PR is only a draft right now due to a bug with the verify command not finding the allow-warnings flag. The args.is_present("allow-warnings") returns false for some reason.

@jfecher jfecher requested a review from vezenovm November 16, 2022 09:25
@guipublic
Copy link
Contributor

Just to be sure I understand properly; this PR makes the unused variable error as a warning, but by default warnings are treated like an error. Then there is the allow-warning flag to be able to compile with warning?

@jfecher
Copy link
Contributor Author

jfecher commented Nov 16, 2022

That is correct - this PR is also the same as @vezenovm's with just the flag name changed and merge conflicts with master fixed

@jfecher
Copy link
Contributor Author

jfecher commented Nov 16, 2022

I'd also be open to changing the flag name if you have a clearer suggestion

@guipublic
Copy link
Contributor

I'd also be open to changing the flag name if you have a clearer suggestion

allow-warnings for allowing warnings seems like a good name to me!

* specify matching ssubcommand for verify and build

* cargo fmt
@jfecher jfecher marked this pull request as ready for review November 16, 2022 15:42
@vezenovm
Copy link
Contributor

vezenovm commented Nov 16, 2022

I kind of like the flag --dev as it is faster to type, but I am good with --allow-warnings as well. I don't think --dev would be unclear though as it follows the "development" modes that you see in other languages/frameworks where many more warnings are emitted.

guipublic
guipublic previously approved these changes Nov 16, 2022
Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

Good to have this, finally!

@jfecher jfecher merged commit 0e4b675 into master Nov 17, 2022
@jfecher jfecher deleted the jf/unused-warning2 branch November 17, 2022 19:46
TomAFrench added a commit to TomAFrench/noir that referenced this pull request Nov 19, 2022
* master:
  Array sort (noir-lang#477)
  Add `--allow-warnings` flag to treat certain errors as warnings (noir-lang#481)
  Improve field comparison error message (noir-lang#499)
  Disable debug output during integration tests (noir-lang#494)
  Aligns build to (noir-lang#443) Refactor stdlib into a separate noir library (noir-lang#496)
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.

3 participants