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
Introduce rustfmt in a non-invasive manner #1040
Changes from all commits
fd1c658
6461e2d
65d19d9
01471f7
7b50a96
fd217e7
743b197
ee9a3ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,85 @@ | ||
disable_all_formatting = true | ||
# Eventually this shoud be: ignore = [] | ||
ignore = [ | ||
"src/blockdata", | ||
"src/consensus", | ||
"src/network", | ||
"src/util", | ||
] | ||
|
||
hard_tabs = false | ||
tab_spaces = 4 | ||
newline_style = "Auto" | ||
indent_style = "Block" | ||
|
||
max_width = 100 # This is number of characters. | ||
# `use_small_heuristics` is ignored if the granular width config values are explicitly set. | ||
use_small_heuristics = "Max" # "Max" == All granular width settings same as `max_width`. | ||
# # Granular width configuration settings. These are percentages of `max_width`. | ||
# fn_call_width = 60 | ||
# attr_fn_like_width = 70 | ||
# struct_lit_width = 18 | ||
# struct_variant_width = 35 | ||
# array_width = 60 | ||
# chain_width = 60 | ||
# single_line_if_else_max_width = 50 | ||
|
||
wrap_comments = false | ||
format_code_in_doc_comments = false | ||
comment_width = 100 # Default 80 | ||
normalize_comments = false | ||
normalize_doc_attributes = false | ||
format_strings = false | ||
format_macro_matchers = false | ||
format_macro_bodies = true | ||
hex_literal_case = "Preserve" | ||
empty_item_single_line = true | ||
struct_lit_single_line = true | ||
fn_single_line = true # Default false | ||
where_single_line = false | ||
imports_indent = "Block" | ||
imports_layout = "Mixed" | ||
imports_granularity = "Module" # Default "Preserve" | ||
group_imports = "StdExternalCrate" # Default "Preserve" | ||
reorder_imports = true | ||
reorder_modules = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't like these much. Sometimes it may be nicer to logically group the imports, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO "reliable benefit of predictable ordering" > "barely ever used benefit of expressing some logical groups, that also has to be maintained over time (which it won't)". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for what @dpc said. I do understand the desire to have 'pub use' statements separate but its just another implicit policy for devs to have to remember and like dpc said such groupings never get maintained. TBH I'm kind of disheartened by this discussion, taking care of the import statements is, IMO, a no-brainer. If we cannot come to some basic agreement that at some stage its better to use tools than developer time it is really hard to make progress on this PR. Lets not become a project where no changes can be made because there is always some negative in any change - implying that we get stuck with negatives we are unable to address. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I don't care about order being lexicographically sorted much and if there's no setting to put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I remembered one more annoying thing about sorting: if you rename an item then you also have to move it so instead of clear -+ diffl lines being one after another you get them separated. Not saying that it's worth turning off sorting but if there was a nicer solution, that'd be great. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree on the diff lines separated thing. So just to make sure I've got it, we can leave the two 'reorder' flags on and acknowledge that it is a desired improvement to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I was thinking about opening issues at their repo once we agree what we need. |
||
reorder_impl_items = false | ||
type_punctuation_density = "Wide" | ||
space_before_colon = false | ||
space_after_colon = true | ||
spaces_around_ranges = false | ||
binop_separator = "Front" | ||
remove_nested_parens = true | ||
combine_control_expr = true | ||
overflow_delimited_expr = false | ||
struct_field_align_threshold = 0 | ||
enum_discrim_align_threshold = 0 | ||
match_arm_blocks = false # Default true | ||
match_arm_leading_pipes = "Never" | ||
force_multiline_blocks = false | ||
fn_args_layout = "Tall" | ||
brace_style = "SameLineWhere" | ||
control_brace_style = "AlwaysSameLine" | ||
trailing_semicolon = true | ||
trailing_comma = "Vertical" | ||
match_block_trailing_comma = false | ||
blank_lines_upper_bound = 1 | ||
blank_lines_lower_bound = 0 | ||
edition = "2018" | ||
version = "One" | ||
inline_attribute_width = 0 | ||
format_generated_files = true | ||
merge_derives = true | ||
use_try_shorthand = false | ||
use_field_init_shorthand = false | ||
force_explicit_abi = true | ||
condense_wildcard_suffixes = false | ||
color = "Auto" | ||
required_version = "1.5.1" | ||
unstable_features = false | ||
disable_all_formatting = false | ||
skip_children = false | ||
hide_parse_errors = false | ||
error_on_line_overflow = false | ||
error_on_unformatted = false | ||
emit_mode = "Files" | ||
make_backup = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer these to be always on a new line. The practical consequence is that you can quickly comment out things if types match and diffs are easier to understand. I had real-life experience with long iterator chain where types were preserved (lot of
map
s) and could easily experiment with the code by commenting/adding lines.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, there's another practical consequence of such things - people miss context when reviewing/reading code
Cause
Things
End
Up
Too
Vertical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO: If a chain of things is so long that it hinders your ability to see stuff, you have some other problems to address.
Like in this case the whole thing could be some
fn get_current_unix_ts_secs()
, because why would anyone wanted to parse this lower-level 4 element implementation detail, when looking at the higher level code.It's an example code, so 🤷 , but this whole function could be more idiomatically:
with comments about what each thing means on the builder methoder, instead of each variable, and 1/4 of the LoC size, while being much easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW. I wish Rust just made a typed builder a language thing, because in the absence of optional and named arguments, builders are a common necessity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO
chain_width
should really be 100, I thought it was a concrete line length not % lol.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really, really, really dont understand the rust fascination with deliberately ensuring code is as
vertical
as
is
at
all
possible
to
make
sure
you
lose
all
context.
If we say we want lines up to 100 chars long, that should include chains, I'm not sure why chains are in any way special?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so that is a vote for
And remove all the granular width configuration options, I don't believe we had previously considered that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, neither is great and if there even exists optimal line, I don't think it can be expressed in code. So I give up on this discussion, do as you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: Deleted my comment after seeing more discussion on this below.