Better rustfmt #51

AaronKutch · 2019-09-23T02:48:07Z

I compared the .rustfmt.toml I made with yours and I am suggesting a few changes. I placed the keys in alphabetical order. I think using imports_layout = "HorizontalVertical" is better since it uses horizontal layout for 2-4 imports (typically) and a vertical layout for longer than that.
I think keeping comment width at 80 is a good idea, however I would prefer the default max_width = 100. I also used a few nightly keys to improve comment formatting.

coveralls · 2019-09-23T03:13:04Z

Coverage decreased (-0.1%) to 75.83% when pulling 494faba on AaronKutch:new_rustfmt into 10aa554 on Robbepop:master.

codecov-io · 2019-09-23T03:45:56Z

Codecov Report

Merging #51 into master will decrease coverage by 0.34%.
The diff coverage is 34.82%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   76.12%   75.77%   -0.35%     
==========================================
  Files          22       21       -1     
  Lines        5340     5288      -52     
==========================================
- Hits         4065     4007      -58     
- Misses       1275     1281       +6

Impacted Files	Coverage Δ
src/uint.rs	`0% <ø> (ø)`	⬆️
src/bitwidth.rs	`89.06% <ø> (-9.51%)`	⬇️
src/storage.rs	`100% <ø> (ø)`	⬆️
src/digit_seq.rs	`77.77% <ø> (ø)`	⬆️
src/apint/casting.rs	`77.72% <ø> (+1.33%)`	⬆️
src/apint/serde_impl.rs	`78.94% <ø> (ø)`	⬆️
src/bitpos.rs	`100% <ø> (ø)`	⬆️
src/apint/shift.rs	`100% <ø> (ø)`	⬆️
src/int.rs	`0% <ø> (ø)`	⬆️
src/radix.rs	`88.88% <ø> (ø)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10aa554...494faba. Read the comment docs.

Robbepop · 2019-09-23T08:04:10Z

I compared the .rustfmt.toml I made with yours and I am suggesting a few changes. I placed the keys in alphabetical order. I think using imports_layout = "HorizontalVertical" is better since it uses horizontal layout for 2-4 imports (typically) and a vertical layout for longer than that.
I think keeping comment width at 80 is a good idea, however I would prefer the default max_width = 100. I also used a few nightly keys to improve comment formatting.

Can you please precisely list what changes you made exactly?
I was even thinking about setting max_width to 80 but that would have been too disruptive for now. In my opinion long lines are just another indicator of code smell. Also I do not want an ultra-flat screen for code review or for looking at diffs.

I also find it easier to look at horizontal lists to grab an overview that's why I chose imports_layout = "Vertical" but I might go with you. Let me think about it.

AaronKutch · 2019-09-23T16:15:44Z

I decided to go with your max_width = 90. The differences between my .rustfmt.toml and the current one is:

imports_layout = "HorizontalVertical"
version = "Two" ( changes barely anything)
error_on_line_overflow = true (I manually formatted in some places to make this work)
error_on_unformatted = true
report_fixme = "Never" (I always look at the todo's manually)
report_todo = "Never"
newline_style = "Unix" # changed, prevents problems when a file is created on Windows
I also added a few new keys such as:
struct_lit_single_line = true
format_macro_bodies = true
format_code_in_doc_comments = true

Robbepop · 2019-09-29T16:41:59Z

My comments on your proposals below.

* imports_layout = "HorizontalVertical"

I will think about this.
I might prefer this over Vertical if we can manage to reduce the line noise to line widths of at most 40 columns or so. I really hate to read too many comma-separated names in a single line.

* error_on_line_overflow = true (I manually formatted in some places to make this work)
* error_on_unformatted = true

Maybe we want this for CI instead since I do not want to pollute too many warnings into errors as I have made the experience of this causing troubles with prototyping new code.

* report_fixme = "Never" (I always look at the todo's manually)
* report_todo = "Never"

My experience in the past with these flags were very positive. But also maybe we want to provide those as errors in CI. I'll think about it.

* newline_style = "Unix" # changed, prevents problems when a file is created on Windows

If this really fixes problem I wonder why it isn't the default. Can you elaborate or link to an issue?

* struct_lit_single_line = true

I like this but it is unstable (as some other already used flags) and since apint is also for the stable channel we have to be careful what configurations of rustfmt we actually are able to use.

* format_macro_bodies = true
* format_code_in_doc_comments = true

I have had bad experience with these in the past and won't accept them therefore.

AaronKutch · 2019-09-30T15:24:10Z

I am using newline_style = "Unix" to enforce LF line endings throughout the codebase. For some reason (probably due to git or one of my editors trying to change LF back to CRLF), I always have a problem with the files being a mix of CRLF and LF. I just checked manually, and some files are still CRLF for some reason. I don't know if it is git's problem or some bug with rustfmt. I just force pushed a commit to fix it.

Maybe we want this for CI instead since I do not want to pollute too many warnings into errors as I have made the experience of this causing troubles with prototyping new code.

I only found about five places in the entire codebase that rustfmt was unable to format, mainly due to comments in the middle of method chains.

My experience in the past with these flags were very positive. But also maybe we want to provide those as errors in CI. I'll think about it.

These should be put in CI.

I like this but it is unstable (as some other already used flags) and since apint is also for the stable channel we have to be careful what configurations of rustfmt we actually are able to use.

rustfmt is purely a developer side thing, so I don't think there is any issues with the unstable flags.

I have had bad experience with these in the past and won't accept them therefore.

I have experienced only positive things, except for the documentation for from_str_radix in serialization.rs, where I had to change the comments from being inline to being in front of what they are commenting on

AaronKutch · 2019-09-30T20:48:31Z

Github has a feature to view all non-whitespace changes

AaronKutch · 2019-10-01T15:56:40Z

I fixed the merge conflicts. The Fix rustfmt errors contains all the manual reformatting.

AaronKutch · 2019-10-01T16:00:29Z

There are warnings after the rebase that appear to be from your commits. I will fix them sometime next week.

AaronKutch · 2019-12-07T23:01:59Z

Are you still not satisfied with my changes?

Robbepop · 2019-12-07T23:10:47Z

Are you still not satisfied with my changes?

Have to give it another look.
Didn't look into this for a while.
But what I can quickly say is that I definitely find

use crate::{
    apint::ApInt,
    bitwidth::BitWidth,
    digit::Digit,
};

... more readable than ...

use crate::{apint::ApInt, bitwidth::BitWidth, digit::Digit};

AaronKutch · 2019-12-07T23:50:03Z

Should I squash the last three commits?

AaronKutch · 2019-12-07T23:55:57Z

wait, I just realized I need to rebase on top of the into_iter change, let me fix it

AaronKutch · 2019-12-08T00:21:43Z

That should be it, except I am getting the warning

warning: unused import: `libm::F64Ext`
   --> src\apint\serialization.rs:277:13
    |
277 |         use libm::F64Ext as _

I presume this is from the commit that supports no_std

Robbepop · 2019-12-08T08:19:42Z

That should be it, except I am getting the warning
warning: unused import: `libm::F64Ext`
   --> src\apint\serialization.rs:277:13
    |
277 |         use libm::F64Ext as _
I presume this is from the commit that supports no_std

Then maybe we simply need a cfg no_std for the import.

AaronKutch · 2019-12-08T20:40:57Z

I fixed the warning for most cases and a compilation error for serde_support with no_std. For some bizarre reason, when I run with cargo check --features=serde_support,libm_0 --no-default-features, it gives an unused warning as if the F64Ext isn't needed any more. I cannot see anywhere where adding serde_support pulls in a way for the log2 function to not need the F64Ext in no_std. It doesn't do the same thing with rand_support.

AaronKutch · 2019-12-19T16:48:42Z

I am almost done with getting my division algorithms into compiler-builtins. When are you going to merge this?

Robbepop · 2019-12-19T16:53:35Z

I am almost done with getting my division algorithms into compiler-builtins. When are you going to merge this?

Sorry, forgot about this PR.
Can you send me a link to your compiler-builtin PR?
I will review this again and eventually merge this today or tomorrow - I will keep the TAB open so that I don't forget again. ;)

AaronKutch · 2019-12-19T17:01:57Z

rust-lang/compiler-builtins#332 This will greatly improve u128 division and divisions for 32 bit platforms

AaronKutch · 2019-12-25T03:47:59Z

All I want for Christmas is a rapid PR review cycle

Robbepop · 2019-12-25T10:17:37Z

.rustfmt.toml

+force_multiline_blocks = true # help certain closures
+format_code_in_doc_comments = true # changed
+format_macro_bodies = true # changed
+format_macro_matchers = true # changed


This caused problems in the past.

I have unchanged them all.

Robbepop · 2019-12-25T10:18:30Z

.rustfmt.toml

+format_code_in_doc_comments = true # changed
+format_macro_bodies = true # changed
+format_macro_matchers = true # changed
+format_strings = true # changed


I think this is not a good default to have. Why would you want that?

I haven't had any bad experiences with those settings, except for changing the force_multiline_blocks = true setting which was actually better with its default value. I can't remember my reasoning from a few months ago.

Robbepop · 2019-12-25T10:20:37Z

.rustfmt.toml

-imports_layout = "Vertical" # changed
+imports_layout = "Vertical" # makes editing quicker
+indent_style = "Block"
+inline_attribute_width = 0 # unchanged, for ease of editing


This option is unstable and since we only set it to its default pls remove.

Robbepop · 2019-12-25T10:25:09Z

.rustfmt.toml

+report_fixme = "Never"
+report_todo = "Never"


We should actually report them always. At least I would want that because I would not like to merge any PRs that actually have those in them. Instead convert TODO and FIXME into proper issues with proper description before merging a PR. This is a really nice to have warning.

Robbepop · 2019-12-25T10:29:51Z

.rustfmt.toml

+error_on_line_overflow = true # be more strict
+error_on_unformatted = true # be more strict


Error if Rustfmt is unable to get all lines within max_width, except for comments and string literals. If this happens, then it is a bug in Rustfmt.

Reeeeeally not sure if I want our CI to fail because of a bug in rustfmt ...

There was a bug I encountered (rust-lang/rustfmt#3805), but it was easily fixable by changing the signature width.

Robbepop · 2019-12-25T10:35:29Z

All I want for Christmas is a rapid PR review cycle

You can help this rapid PR review cycle by chunking your commits in a way that makes them easy to review. For example in this PR the single most important file is obviously .rustfmt.toml. However, you made two changes to it: Changing some options and changing ordering: The first change is severe, the other change is trivial to review. However, since you merged both changes into a single commit it made it hard to extract the severe parts out of the many trivial ones. So by simply phasing out those 2 changes into two separate commits you wouldn't force a reviewer to go through every single line, check what changed and if so check the Rust docs but would simply pre-filter the important parts out.

AaronKutch · 2019-12-25T16:53:15Z

However, since you merged both changes into a single commit it made it hard to extract the severe parts out of the many trivial ones.

What happened here was I completely replaced the existing rustfmt.toml with my original rustfmt.toml I had made for the reorganization PR that never made it. I should have put yours in alphabetical order in one commit, and then replaced it with another commit.

AaronKutch · 2019-12-26T14:56:41Z

I had a really bad sickness recently and it sapped all my energy and is distorting my judgement. I will redo the commits a third time to fix your problems

AaronKutch · 2019-12-26T16:35:27Z

I split up the formatting itself to show you what the

format_code_in_doc_comments = true # changed
format_macro_bodies = true # changed
format_macro_matchers = true # changed
format_strings = true # changed

part does so you can see it is useful

AaronKutch · 2019-12-26T16:36:56Z

cargo fmt returns nothing except for TODO warnings after the last commit.

Robbepop · 2019-12-26T21:03:39Z

I had a really bad sickness recently and it sapped all my energy and is distorting my judgement. I will redo the commits a third time to fix your problems

It is fine. I have already went through the whole changes. I will give it another look tomorrow (very tired rn) and probably approve and merge then. Thank you again for all your work on this crate!

Robbepop

This is perfect now and due to the new commit history super easy to review! Thank you!

AaronKutch force-pushed the new_rustfmt branch from 9780ecc to b4ed7ab Compare September 23, 2019 16:04

AaronKutch force-pushed the new_rustfmt branch from b4ed7ab to 08bb1fd Compare September 30, 2019 15:24

AaronKutch force-pushed the new_rustfmt branch from 08bb1fd to 4a31e0a Compare October 1, 2019 15:55

AaronKutch force-pushed the new_rustfmt branch from 8d810c1 to 4320db5 Compare December 8, 2019 00:20

Robbepop reviewed Dec 25, 2019

View reviewed changes

AaronKutch added 2 commits December 26, 2019 09:56

Reorder .rustfmt.toml in alphabetic order

3dbab4d

Update .rustfmt.toml

1fd0c64

AaronKutch added 3 commits December 26, 2019 10:11

Run rustfmt 1.4.9

442e0ee

Make more changes to .rustfmt.toml and run it

0cdaa5a

Manual format to get into valid state

494faba

AaronKutch force-pushed the new_rustfmt branch from 9e245dd to 494faba Compare December 26, 2019 16:32

Robbepop approved these changes Dec 27, 2019

View reviewed changes

Robbepop merged commit b161255 into Robbepop:master Dec 27, 2019

AaronKutch deleted the new_rustfmt branch December 27, 2019 21:27

		error_on_line_overflow = true # be more strict
		error_on_unformatted = true # be more strict

Better rustfmt #51

Better rustfmt #51

Conversation

AaronKutch commented Sep 23, 2019

coveralls commented Sep 23, 2019 • edited Loading

codecov-io commented Sep 23, 2019 • edited Loading

Codecov Report

Robbepop commented Sep 23, 2019

AaronKutch commented Sep 23, 2019

Robbepop commented Sep 29, 2019 • edited Loading

AaronKutch commented Sep 30, 2019 • edited Loading

AaronKutch commented Sep 30, 2019

AaronKutch commented Oct 1, 2019

AaronKutch commented Oct 1, 2019

AaronKutch commented Dec 7, 2019

Robbepop commented Dec 7, 2019

AaronKutch commented Dec 7, 2019

AaronKutch commented Dec 7, 2019

AaronKutch commented Dec 8, 2019 • edited Loading

Robbepop commented Dec 8, 2019

AaronKutch commented Dec 8, 2019 • edited Loading

AaronKutch commented Dec 19, 2019

Robbepop commented Dec 19, 2019 • edited Loading

AaronKutch commented Dec 19, 2019

AaronKutch commented Dec 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop commented Dec 25, 2019

AaronKutch commented Dec 25, 2019

AaronKutch commented Dec 26, 2019

AaronKutch commented Dec 26, 2019

AaronKutch commented Dec 26, 2019

Robbepop commented Dec 26, 2019

Robbepop left a comment

Choose a reason for hiding this comment

coveralls commented Sep 23, 2019 •

edited

Loading

codecov-io commented Sep 23, 2019 •

edited

Loading

Robbepop commented Sep 29, 2019 •

edited

Loading

AaronKutch commented Sep 30, 2019 •

edited

Loading

AaronKutch commented Dec 8, 2019 •

edited

Loading

AaronKutch commented Dec 8, 2019 •

edited

Loading

Robbepop commented Dec 19, 2019 •

edited

Loading