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

Jump to bottom

#26972: Create CI task to ensure that all Rust files have been formatted with rustfmt #265

Closed
wants to merge 5 commits into from
Closed

#26972: Create CI task to ensure that all Rust files have been formatted with rustfmt #265

wants to merge 5 commits into from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@teor2345 @coveralls
@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 8, 2018

No description provided.

rust: note missing_docs
a844781 
Commits af182d4 and
b605929 tried to enforce
mandatory documentation here, but missed the !.
teor2345
teor2345 reviewed Aug 8, 2018
View changes
Copy link
Contributor Author

@teor2345 teor2345 left a comment

Please see my comments on the pull request. We just need a few minor tweaks, then we'll be ready to merge.

Makefile.am Outdated
rustfmt:
if USE_RUST
@if test -x "`which cargo-fmt 2>&1;true`"; then \
(cd "$(top_srcdir)/src/rust" && cargo fmt --all --); \

This comment has been hidden.

Sign in to view

Makefile.am
else \
echo "Tor uses rustfmt (via cargo-fmt) to format Rust code."; \
echo "However, it seems that you don't have rustfmt installed."; \
printf "You can install rustfmt by following the directions here:"; \

This comment has been hidden.

Sign in to view

Makefile.am Outdated
else \
echo "Tor uses rustfmt (via cargo-fmt) to format Rust code."; \
echo "However, it seems that you don't have rustfmt installed."; \
printf "You can install rustfmt by following the directions here:"; \

This comment has been hidden.

Sign in to view

Makefile.am Outdated
@if test -x "`which cargo-fmt 2>&1;true`"; then \
(cd "$(top_srcdir)/src/rust" && cargo fmt --all -- --check) || \
(echo "**************** check-rustfmt failed. ****************"; \
echo " Run \`make rustfmt\` to apply the above changes."); \
Copy link
Contributor Author

@teor2345 teor2345 Aug 8, 2018

make should fail if check-rustfmt fails, like all the other check targets

src/rust/.rustfmt.toml Outdated
@@ -1,2 +1,12 @@
max_width = 80
Copy link
Contributor Author

@teor2345 teor2345 Aug 8, 2018

Please use the default Rust max_width, see https://trac.torproject.org/projects/tor/ticket/27071

src/rust/build.rs
@@ -10,14 +10,12 @@
use std::collections::HashMap;
use std::env;
use std::fs::File;
use std::io::prelude::*;

This comment has been hidden.

Sign in to view

src/rust/protover/protover.rs Outdated
@@ -624,7 +624,7 @@ impl ProtoverVote {
}

for (protocol, versions) in vote.iter() {
let supported_vers: &mut HashMap<Version, usize> =
let supported_vers: &mut HashMap<_, usize> =
Copy link
Contributor Author

@teor2345 teor2345 Aug 8, 2018

If we're using the default max_width, we might not need this change.

Makefile.am Outdated
check-rustfmt:
if USE_RUST
@if test -x "`which cargo-fmt 2>&1;true`"; then \
(cd "$(top_srcdir)/src/rust" && cargo fmt --all -- --check) || \
Copy link
Contributor Author

@teor2345 teor2345 Aug 8, 2018

Before running rustfmt --check, please echo a message saying what rustfmt --check does.

For example, check-typos says:
echo "Checking for Typos ..."; \

@coveralls
Copy link

@coveralls coveralls commented Aug 8, 2018 •
edited

Coverage Status

Coverage remained the same at 59.52% when pulling 5d749e4 on teor2345:rustfmt-travis into 837f11a on torproject:master.

@teor2345 teor2345 force-pushed the rustfmt-travis branch from 80d4554 to 31af502 Aug 8, 2018
cypherpunks added 4 commits Aug 8, 2018
rust: max_width=100, other rustfmt settings. #27071
feaa7c3 
These are the 12 stable and documented configuration options,
set to their default values.

use_small_heuristics is only stabilized in rustfmt 0.9, so maintain
support for 0.8.x for now by commenting it out.

comment_width is unstable and did nothing, since wrap_comments defaults
to false.

Default values gotten from `rustfmt --print-config default rustfmt.toml`.

https://github.com/rust-lang-nursery/rustfmt/blob/e7932fa9c2591c45a37a24305de90cb63128afcf/Configurations.md
add make check-rustfmt
4913d72
rust: run rustfmt
7fb342a
rust: add check-rustfmt to make check. #26972
5d749e4
@teor2345 teor2345 force-pushed the rustfmt-travis branch from 31af502 to 5d749e4 Aug 8, 2018
@teor2345
Copy link
Contributor Author

@teor2345 teor2345 commented Aug 16, 2018

Obsoleted by #275.

@teor2345 teor2345 closed this Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment