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

Add a default rustfmt to project root #69

Closed
wants to merge 2 commits into from

Conversation

ishai1
Copy link

@ishai1 ishai1 commented Dec 30, 2024

My attempt at a function which will create a default rustfmt file in the current project - following up on #68.

@CeleritasCelery
Copy link
Contributor

Thanks for creating this PR. Why did you pick these particular settings for the rustfmt file?

@ishai1
Copy link
Author

ishai1 commented Dec 31, 2024

The default from copilot. At a quick glance it seemed reasonable but I don't have good insight in to what makes a good rustfmt config so won't pretend it's better than any other config.

@CeleritasCelery
Copy link
Contributor

I don't think we would want to specify rustfmt settings for the user unless there is a de facto standard that most people would want. I would assume that the default settings (i.e. no rustfmt file) would be the most common.

@ishai1
Copy link
Author

ishai1 commented Dec 31, 2024

Fair enough, I think it could stil be nice to provide users with a basic rustfmt file. I changed to create a file with a single entry for edition which is defined to its default value of "2015" - https://rust-lang.github.io/rustfmt/?version=master&search=#edition.

I'll close the PR if it still doesn't seem appropriate to you 🙂.

@CeleritasCelery
Copy link
Contributor

From the docs you linked:

Rustfmt is able to pick up the edition used by reading the Cargo.toml file if executed through the Cargo's formatting tool cargo fmt.

If we specify 2015 in the rustfmt toml file it will override what the user has in their cargo.toml. Which is probably not what user wants. Especially since that is a really old edition that realistically few people are using.

@ishai1
Copy link
Author

ishai1 commented Jan 2, 2025

Oh, good point! That is a pointer to how I should have solved this problem in the first place. Specifically changing rustic-rustfmt-bin to cargo and rustic-rustfmt-args to fmt will then pick up the version from cargo.toml.

@ishai1 ishai1 closed this Jan 2, 2025
@ishai1 ishai1 deleted the generate-rustfmt-toml branch January 2, 2025 19:34
@ishai1
Copy link
Author

ishai1 commented Jan 2, 2025

Thank you for the patient review! 🙂

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.

2 participants