should_report_error: don't report error when using stdin

[brotzeit]

I'm not sure what's the best way to fix those tests.

[topecongiro]

Thank you for your PR! The code looks good to me, would you please rebase against the latest master?

[brotzeit]

Ok. But I'm still not sure how to handle those tests as they are using stdin.

#[test]
fn format_lines_errors_are_reported() {
    let long_identifier = String::from_utf8(vec![b'a'; 239]).unwrap();
    let input = Input::Text(format!("fn {}() {{}}", long_identifier));
    let config = Config::default();
    let (error_summary, _file_map, _report) =
        format_input::<io::Stdout>(input, &config, None).unwrap();
    assert!(error_summary.has_formatting_errors());
}

Do I have to create a temp file instead or what could be a good solution here ?

[topecongiro]

Oh, I was misunderstanding what this PR is trying to do.

I think that when we are using stdin and fail to format, we should not emit error report (e.g. line exceeded max width stuffs) but the exit code should still be non-zero. Could you please update this PR that way? This could be done by performing the similar check inside run().

[brotzeit]

I will give it a try.

[brotzeit]

I have thought about this and I honestly don't understand why we should do this. IMO we should either ignore the error completely with stdin or we emit the error report and return a non-zero exit code.

The emacs package for rust makes use of stdin. And when I understand this correctly, this change would trigger an error but wouldn't return any error message. How should I know what's the reason for the error ? Or am I missing something here ?

[nrc]

What's the original motivation for this PR? It does seem to me that even if using stdin, you'd want to see the error report on stdout

[brotzeit]

What's the original motivation for this PR?

The error report is emitted together with the formatted string.

But I think it would be better if we return an exit code for this kind of errors like @topecongiro said.

[nrc]

So, IIUC, the underlying problem here is not the output but the exit code. When #2539 lands, then we will have update exit behaviour and will get 0 whether reformatting happens or not and 1 if there was an internal error (not a difference in formatting). Then, in stdin mode, we can emit the formatted code in the first case and an error message in the second case and clients like emacs can look at the exit code to determine if they should replace source code (0) or report an error to the user (1).

Does that sound good?

[brotzeit]

I think this should be obsolete with the new changes. Thanks.