don't reformat with errors unless --force flag supplied #4256
Conversation
calebcartwright
force-pushed
the
err-invalid-attrs
branch
from
352bf91 to
b4ba91e
Compare
There was a problem hiding this comment.
Thank you for the PR!
I would like to add a command-line flag that would ignore errors in general and format inputs, rather than enabling/disabling each error with command line arguments.
- By default, rustfmt does not format inputs if there were formatting/operation errors
- When
-f,--forcecommand line flag is passed, rustfmt formats inputs regardless of formatting errors
Note that what we treat as an error depends on the configuration:
- By default
DeprecatedAttrandBadAttrare the only errors that should be considered as a formatting error - If the
license_template_pathis used, above plusLicenseCheck - If the
error_on_line_overflowis used, above plusLineOverflow - If the
error_on_unformattedis used, above plusTrailingWhitespace - As
MacroFormatErroris internal, it should be ignored in any case
Ah okay. I was looking at #3989 only in the context of bad attributes, but the holistic error handling you've articulated sounds great. I'll update this accordingly |
calebcartwright
force-pushed
the
err-invalid-attrs
branch
from
b4ba91e to
e13b5fa
Compare
calebcartwright
changed the title
calebcartwright
force-pushed
the
err-invalid-attrs
branch
from
e13b5fa to
3165dc8
Compare
calebcartwright
force-pushed
the
err-invalid-attrs
branch
from
3165dc8 to
ff96119
Compare
|
@topecongiro this should be ready for review whenever you get a chance. I only have two outstanding thoughts/questions:
|
| @@ -413,6 +416,22 @@ fn format_string(input: String, opt: Opt) -> Result<i32> { | |||
| verbosity: Verbosity::Quiet, | |||
| }; | |||
| let report = rustfmt_nightly::format(Input::Text(input), &config, setting)?; | |||
|
|
|||
| if report.has_errors() { | |||
There was a problem hiding this comment.
I think that we don't want to emit errors in stdin mode by default, as it clutters the output.
There was a problem hiding this comment.
Fair enough, though wouldn't that result in rustfmt silently exiting with an error code in such circumstances?
There was a problem hiding this comment.
My bad, what I meant was when running in force mode, we do not want to emit the error report because both error reports and the formatting result will be printed.
To summarize the behavior of the stdin mode when executed in conjunction with multiple flags,
rustfmt # Print error message
rustfmt --quiet # No error message
rustfmt --verbose # Print error message
rustfmt --force # No error message
rustfmt --force --quiet # No error message
rustfmt --force --verbose # Print error messageThere was a problem hiding this comment.
Ah okay, that makes sense!
There was a problem hiding this comment.
Thank you so much for the updates; feels great to review the PR with a bunch of tests 😄
I commented on whether or not rustfmt should print the error message, but after reviewing the existing code, it seems that we don't take the flags like --quiet and --verbose into account properly in the first place.
I think we should sort out the spec of these flags on each mode and fix them in a different PR rather than rushing the fix. As such, this PR LGTM as is.
Closes #3989