refactor - improved error regarding issue "Better error message #6302"

[rufevean]

closes #6302

[rufevean]

i cant figure out why my tests are failing, could you please guide me through this?

[ding-young]

i cant figure out why my tests are failing, could you please guide me through this?

It looks like the self-tests didn't pass. Since it's important for the rustfmt source code to adhere to the same formatting style that it enforces, the self-test runs rustfmt on its own codebase. You can find more details in the Contributing.md.

To resolve this, you might want to try running cargo run --bin rustfmt /path/to/modified/source.rs to format the modified source code using the rustfmt built from the source. After that, you can check if the tests pass locally by running cargo test.

Hope this helps!

[rufevean]

@ytmimi i think its ready for review now, thank you

[rufevean]

Thanks for continuing to look into this. I've left a few comments inline. Also, I think it would be great if we could add a unit test similar to those defined here, where we call the rustfmt binary and check the content of stdout / stderr.

In this case we can create a new config file in tests/config that will trigger the error, and we can call rustfmt with the --config-path argument and validate that we see our expected error message in stderr.

okay, so we have to write a test case in tests/rustfmt/main.rs that takes the file we create in tests/config which is faulty and we should expect the output as an error, right?

Also, one more note. The rustfmt team strongly discourages merge commits. Please rebase your branch to bring it up to date when you get a chance.

would it be better if i do once we have finalized all the refactoring?

[ytmimi]

okay, so we have to write a test case in tests/rustfmt/main.rs that takes the file we create in tests/config which is faulty and we should expect the output as an error, right?

Yeah, thats right.

would it be better if i do once we have finalized all the refactoring?

Sure. Just wanted to make you aware of our stance on merge commits.

[rufevean]

@ytmimi , why trying to test this issue, i faced two issues

1. when we raise the error , we get

The file `/home/rufevean/shitbox/rustfmt/tests/config/issue-6302.toml` failed to parse.
Error details: invalid type: integer `2019`, expected string
in `edition`

we get the entire path . so we cant test it that way as the absolute path will change for every person. so i think we might have to refactor the code where it will only present the path from the main project directory?

2. in the error display

The file `/home/rufevean/shitbox/rustfmt/tests/config/issue-6302.toml` failed to parse.
Error details: invalid type: integer `2019`, expected string
 in `edition`

the in edition part is forming a new line , to track that issue we might have to tract the {e} directly as of my knowledge, we didnt specify anything that makes it in a new line.

[ytmimi]

we get the entire path . so we cant test it that way as the absolute path will change for every person. so i think we might have to refactor the code where it will only present the path from the main project directory?

How exactly are you trying to test this? I think it should be doable. If you could add a commit with your test code that would be helpful for me to review and provide some suggestions.

the in edition part is forming a new line , to track that issue we might have to tract the {e} directly as of my knowledge, we didnt specify anything that makes it in a new line.

If you look at the original issue the in edition was also on a newline. It's maybe not that visually appealing, but I don't think it's something we need to address right now.

[rufevean]

@ytmimi , i have added the commit, could you please check it and let me know where i am getting it wrong?

[rufevean]

Let me know if you'd like to squash the commits / rebase the PR or if you'd rather I just squash the commits on the merge.

i had left a comment regarding Ok(parsed_config.to_parsed_config(style_edition, edition, version, file_path.parent().unwrap_or(Path::new("")))) and i would like to know how do you want me to proceed, and once i have refactored it according to how you think is better. i will rebase it . thank you

[rufevean]

@ytmimi can we rebase and merge this now?

[rufevean]

@ytmimi i think i did something wrong, i rebased it and squashed all my commits into 1. but i can see other's commits here .

[ytmimi]

Yeah, definitely doesn't look right. You should be able to get back to the commit before the rebase if you check the git reflog

[rufevean]

@ytmimi , i tried squashing it again and deleting the commits that i dont want to include but even then there are some conflicts . its better if you may handle it. thank you

[ytmimi]

hmm weird, when I tried to update your branch GitHub marked me as closing the PR 🤔