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

Apply rustfmt formatting (partial) #926

Merged
merged 24 commits into from
Jan 8, 2020
Merged

Apply rustfmt formatting (partial) #926

merged 24 commits into from
Jan 8, 2020

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jan 2, 2020

Rustfmt is now widely used and mostly gives a good result. This PR:

  • accepts changes which are reasonable (some a clear improvement, some neither much better nor worse)
  • explicitly skips formatting in a few cases
  • applies manual reformatting in a few cases (to bring formatting closer to rustfmt's style)
  • thus makes Rand more consistent with typical Rust styles and easier to use rustfmt on

However, some reformatting was rejected without a skip command:

  • in some cases, content is split over too many or too few lines, impairing readability
  • some of the arrays used in tests are problematic
  • there is an incident of incorrect operator binding priority: index..=index + 1
  • some vertical alignment is lost (I would just use rustfmt::skip, but it can't be applied to if expressions or inside blocks)

I have created several new issues on the rustfmt repository; ideally the remaining formatting issues can be resolved via rustfmt improvement.
For now, we still can't just run cargo fmt on the project.

Copy link
Member Author

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Since no one else has reviewed this and I'm not "allowed" to self-review (GH restriction), I'm just going to make some comments and merge anyway:

  • I'm pretty sure there are no semantic changes here (other than to rustfmt config)
  • Syntactic changes are mixed and use a few more lines overall, but for the most part are an improvement
  • Aligning with Rustfmt formatting is (broadly speaking) sensible
  • It's a shame this PR doesn't get all the way there

@dhardy dhardy merged commit e2dc2c3 into rust-random:master Jan 8, 2020
println!("v: {}", v[i]);
e
})
.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this decreases readability, but I guess it's good enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess you're right. Feel free to post rustfmt config changes / a skip annotation if you prefer.

I'd really like to get the point where we can just run cargo fmt on the project so would prefer not to just revert stuff like this — but it may not be until Rustfmt 2.0 that we get there (and even then, we'll have to make further compromises).

@vks
Copy link
Collaborator

vks commented Jan 8, 2020

I think there are some minor readability regressions, where the code is spread over too many lines, but I think this is a small annoyance outweighed by the advantages of automatic formatting.

@dhardy
Copy link
Member Author

dhardy commented Jan 8, 2020

Glad we agree on that then.

Try running cargo fmt locally and see if you have ideas for dealing with that lot :-/

@vks
Copy link
Collaborator

vks commented Jan 8, 2020

Ok, here are my observations with rustfmt 1.4.11-nightly (1838235 2019-12-03):

I think the following changes will hopefully improve in a later rustfmt version:

  • ..a + b should be formatted reflecting the operator precedence (see the issue you opened).
  • Some copy_nonoverlapping calls get reformatted with too much stuff on one line. This is a case were the soft 80 characters limit should be preferred over the hard 100 characters limit. There are a few other similar cases.
  • The ziggurat calls get reformatted in a less readable way.

I think some changes have to be manually disabled by annotating the source code:

  • Arrays of "random" numbers get reformatted, hurting readability by spreading them over more lines. (I remember this being an issue with clang-format as well. I don't think this will ever get fixed.)
  • The alignment of the HC128 code gets broken. The current formatting is non-standard, so I think we have to add an exception if we want to keep it.

In my opinion, the remaining changes suggested by rustfmt are acceptable (even if not great).

This actually looks better:

diff --git a/src/distributions/binomial.rs b/src/distributions/binomial.rs
index c096e4a862..53363c6e99 100644
--- a/src/distributions/binomial.rs
+++ b/src/distributions/binomial.rs
@@ -229,7 +229,7 @@ impl Distribution<u64> for Binomial {
                 }
 
                 if alpha
-                        > x_m * (f1 / x1).ln()
+                    > x_m * (f1 / x1).ln()
                         + (n - (m as f64) + 0.5) * (z / w).ln()

@dhardy
Copy link
Member Author

dhardy commented Jan 9, 2020

Generally I agree, though there are a couple of cases where current formatting deliberately breaks over multiple lines.

Arrays of "random" numbers get reformatted,

I think the only way this type of thing can be done correctly is to weight styles by similarity to previous/next lines as well as a soft length limit.

The alignment of the HC128 code gets broken.

There is some interest in vertical alignment; it's already implemented for a couple of things (e.g. enum values).

This actually looks better:

It appears I got this wrong (but right in the rand_distr equivalent). It's one of the examples I brought up in the operator precedence issue.

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.

None yet

2 participants