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

rust: add rustfmt.toml to set edition = "2018" #4709

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

nfelt
Copy link
Contributor

@nfelt nfelt commented Feb 25, 2021

This makes my emacs formatting work again now that we use async: rust-lang/rustfmt#4454

The ideal solution would probably be if rust-mode were to inspect Cargo.toml and pass the edition from there into rustfmt, but this is a quicker fix than trying to send them that patch. Also they previously "fixed the glitch" by just defaulting rust-mode to 2018 edition so I'm not sure they would even take the patch; I still ran into this issue since internal to Google rust-mode is outdated. Besides, it seems technically more correct to have the rustfmt edition derived from the repo rather than set to a global default that happens to work for our project.

@nfelt nfelt added the core:rustboard //tensorboard/data/server/... label Feb 25, 2021
@nfelt nfelt requested a review from wchargin February 25, 2021 22:33
@google-cla google-cla bot added the cla: yes label Feb 25, 2021
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they previously "fixed the glitch" by just defaulting rust-mode to 2018 edition

Scare quotes indeed, since that’s a breaking change; it’s very much
intentional that editions default to 2015 in Rustc/Cargo/etc. In any
case, this change is fine with me, and happy to accommodate.

@@ -17,6 +17,7 @@
name = "rustboard"
version = "0.4.0-alpha.0"
authors = ["The TensorFlow Authors <tensorboard-gardener@google.com>"]
# Keep in sync with `edition` in rustfmt.toml.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it’s worth, also Bazel targets. (No action required.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thanks for pointing that out; will leave as-is since I think rustfmt.toml is the place that's more likely to be forgotten.

@nfelt nfelt merged commit af8c40b into tensorflow:master Feb 25, 2021
@nfelt nfelt deleted the rustfmt-toml branch February 25, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes core:rustboard //tensorboard/data/server/...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants