feat: run rustfmt after generating schema.rs file

[AndreCostaaa]

Closes #4405 by launching a rustfmt process to format the generated schema.rs file

If rustfmt doesn't exist or it throws an error it will simply print a message to the user

[weiznich]

Thanks for opening this PR. It seems to work, but now we need to update the expected output for some tests. You can run the tests locally by using cargo xtask run-tests and then you can use cargo insta review to review the snapshots of the expected output. The later command requires to install cargo-insta.

[weiznich]

Thanks for the update, but the CI still indicates that there is something wrong with the snapshots. It would be great if you could have a look at that, otherwise I will try to address that after the holidays.

[AndreCostaaa]

Thanks for the update, but the CI still indicates that there is something wrong with the snapshots. It would be great if you could have a look at that, otherwise I will try to address that after the holidays.

I've now realized my implementation has a problem.
The print_schema tests use the same snapshots and since i've only ran rustfmt for migration runs we need to either:

1. Separate print_schema and migration tests (don't really like this idea)
2. print-schema should also format its output, specifically the function output_schema inside print_schema.rs

So what I'm thinking is adding a new function output_schema_and_format that calls output_schema and formats the code using this new function on both print_schema and migration run

We can also move this to output_schema but I like the option of keeping the current output_schema function for now. Let me know what you think

[Mingun]

I think, it would be better to run formatter only when explicitly requested by flag. Or at least provide a flag which disables autoformatting.

[AndreCostaaa]

I think, it would be better to run formatter only when explicitly requested by flag. Or at least provide a flag which disables autoformatting.

Sounds like a good idea. Can I assume autoformating is something that we want enabled by default?

[weiznich]

I wouldn't want to introduce an option for formatting.

As for the print-schema "issue": The correct solution is to run formatting only there by using rustfmt in such way that the content of the generated schema is provided via stdin and the formatted code is read from stdout.

[AndreCostaaa]

Here we go. Finally got it working

[weiznich]

Thanks, looks good now 🎉

[AndreCostaaa]

So the CI tests failed before with these errors:

STDERR: error: 'rustfmt' is not installed for the toolchain 'beta-x86_64-unknown-linux-gnu'.
To install, run rustup component add rustfmt

So it made me realize that the rustfmt binary may exist but just as a placeholder (?). Correct me if i'm wrong

Pushed a fix (9c2b8f1 and 5edd2a5) where I check for the rustfmt exit code and return an error in case it's not succesful. Should've done this earlier but didn't realize it was a possibility

[AndreCostaaa]

Perfect. So it seems to be working (as in, it does what I want it to do) but the tests will fail because it tries to check snapshots created with rustfmt vs results created in an environment where rustfmt doesn't exist

I see two options:

1. Add rustfmt to the CI tests environnement
2. Create snapshots with and without formatting and make the test compare with the correct snapshot depending on environment
3. Abandon this PR as it seemed like a small feature at first but I guess not so much by now 😆

I would be happy to work on this until we can make it work. Let me know what you think

[weiznich]

Thanks for this update. It's reasonable to just install rustfmt in the CI. The right point to do this is there:

https://github.com/diesel-rs/diesel/blob/master/.github/workflows/ci.yml#L175

That should be as simple as adding the rustfmt component as documented here: https://github.com/dtolnay/rust-toolchain

[AndreCostaaa]

Thank you for your help, let's watch those tests turn green :)