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

chore: conservatively use log::info instead of println for some expressions #3766

Merged
merged 9 commits into from
Dec 11, 2023

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented Dec 11, 2023

Description

If you want to see info level logs, you can do NOIR_LOG=info nargo compile by default if that environment variable is not set, then it will only print out error level logs

Problem*

Resolves

Summary*

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@kevaundray
Copy link
Contributor Author

I'll open an issue to standardize the categories of logging we have -- some logging like printing out ACIR should be done using println, though for things like "Constraint system successfully built" its a bit trickier because its informational (we don't want it info level logs to be printed out by default), but we also want it to be printed out to tell the user that their program was built successfully

@kevaundray
Copy link
Contributor Author

kevaundray commented Dec 11, 2023

There might be something inherently wrong with my assumption in that we want things like "constraint system built successfully" to always be printed out -- I vaguely remember an issue where someone said that it was not easy to see what had happened.

Another solution is to just create another logging level "feedback" and put these under that category

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

I'm ok with this. I'm a little concerned about hiding some of the warnings related to interacting with backends by default but we can see how that goes to see if we get a resurgence of user questions.

@TomAFrench TomAFrench added this pull request to the merge queue Dec 11, 2023
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

I'm alright with this for now as well. We can re-evaluate it later if needed

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 11, 2023
@TomAFrench TomAFrench added this pull request to the merge queue Dec 11, 2023
Merged via the queue into master with commit 0a23d42 Dec 11, 2023
34 checks passed
@TomAFrench TomAFrench deleted the kw/switch-println-for-log branch December 11, 2023 22:22
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