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

Enable rustfmt #959

Closed
wants to merge 4 commits into from
Closed

Enable rustfmt #959

wants to merge 4 commits into from

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Apr 20, 2022

We have not reached consensus on the usage of rustfmt and there are fair and valid arguments on both sides. Please see the tracking issue for full discussion.

In order to try and resolve the issue of whether or not to use rustfmt I've attempted to come up with a diff that is acceptable to those that dislike certain rustfmt formatting. (Note this PR only adds 1000 lines to the the rust-bitcoin crate which has 26,000 LOC. We are down to 500!)

What I did:

  • Run rustfmt --print-config default rustfmt.toml
  • Make changes to the config file (all changes are commented with the original default value)
  • Manually check the diff and add #[rustfmt::skip] where ever the changes are sub-optimal (by some subjective measure)

Patch 1 is the rustfmt skips. Patch 2 is the result of running cargo +nighly fmt after adding the rustfmt.toml. Reviewers can check mechanically that no other changes were introduced.

Note

We have some stylistic differences in the project, that is ok, we don't all have to agree on everything. Loose consensus and running code is the mantra. This PR does not effect the logic of the code only the formatting - we do not need 100% consensus, and with a binary decision like 'use rustfmt' it is unpractical to expect 100% consensus either way.

Configuration

PR now includes the following configuration, values can be bikeshed'ed:

  • comment width is set to 100 (the predominant current used length in this crate for comments)
  • various other line length options are set to 80 (increased from smaller defaults) to reduce the vertical space used
  • match_arm_blocks = false
  • imports are configured the same way that we recently did in miniscript

@tcharding tcharding changed the title Run rustfmt Enable rustfmt Apr 20, 2022
@tcharding tcharding force-pushed the rustfmt branch 2 times, most recently from 4a8ec80 to 7275409 Compare April 20, 2022 03:04
Copy link
Member

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

I walked through a small bit and left comments on some of the most offensive changes. There's a maintinence burden to not using rustfmt, but I find that with any kind of sensibly careful contributors rustfmt is strictly worse than not, at least if you like to see context around the lines you're writing - something I find to be one of the absolute most important things when reviewing or coding.

One further concern with rustfmt - I actually can't run it! I'm not such an active contributor so its not all that important, but last I heard you can't install rustfmt unless its via rustup, which I don't think is an acceptable thing for any developer working on security-critical software to be using.

@@ -140,7 +138,11 @@ impl BlockHeader {
let mut ret = [0u64; 4];
util::endian::bytes_to_u64_slice_le(block_hash.as_inner(), &mut ret);
let hash = &Uint256(ret);
if hash <= target { Ok(block_hash) } else { Err(BlockBadProofOfWork) }
if hash <= target {
Ok(block_hash)
Copy link
Member

Choose a reason for hiding this comment

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

Bleh.

.unwrap();
let merkle =
Vec::from_hex("bf4473e53794beae34e64fccc471dace6ae544180816f89591894e0f417a914c")
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Bleh, we really need a "dont put unwrap() on its own line" setting :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, this is annoying.

assert_eq!(
real_decode.header.validate_pow(&real_decode.header.target()).unwrap(),
real_decode.block_hash()
);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to not have this have its own line? rustfmt's intense desire to put two characters on their own line is one of the most frustrating things about it for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, this one is really subjective then. For my eyes this is the best formatting for a function call that puts its first argument on a newline.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'd say #959 (comment) is highly subjective, but this one I think you can argue pretty strongly that the two separate indented lines with the two things being checked is pretty clear, and I have a very strong preference for being able to see context while reviewing and working - something I see as really important.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer the rustfmt formatting here, it makes it clear where the call to assert_eq ends. Otherwise it's like reading Python where half the program semantics are invisible.

assert_eq!(format!("{:x}", gen.wtxid()),
"4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b".to_string());
assert_eq!(
format!("{:x}", gen.wtxid()),
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I hate this too, like its not a big change in readability but it makes everything that much more vertical and you lose that much more context...

Copy link
Member

Choose a reason for hiding this comment

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

You can rotate your monitor 90 degrees :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh I already do, it's even 4K. Doesn't mean I ever get as much context as I want :)

@@ -180,7 +180,9 @@ impl fmt::Display for Error {
Error::NumericOverflow => "numeric overflow (number on stack larger than 4 bytes)",
Error::BitcoinConsensus(ref _n) => "bitcoinconsensus verification failed",
Error::UnknownSpentOutput(ref _point) => "unknown spent output Transaction::verify()",
Error::SerializationError => "can not serialize the spending transaction in Transaction::verify()",
Error::SerializationError => {
Copy link
Member

Choose a reason for hiding this comment

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

lol why did it add brackets here, that's just useless

Copy link
Member Author

Choose a reason for hiding this comment

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

Newlines always get brackets I believe. I always assumed this was a counter to how one can leave them off in C when there is a single line in the following block of code and how this was seen by some as a bad thing to do (since its so easy to come back later and add a new line without noticing the scope).

Copy link
Member

Choose a reason for hiding this comment

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

Right, and you can argue that with C because its easy to break, but in Rust (in a match) you can't have a similar issue - if you just blindly add a new statement here its a compile error.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure in Rust you don't actually need the braces -- this is purely a rustfmt thing. I agree that it's silly and unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Now included, thanks.

pub fn new_v1_p2tr<C: Verification>(
secp: &Secp256k1<C>,
internal_key: UntweakedPublicKey,
merkle_root: Option<TapBranchHash>,
Copy link
Member

Choose a reason for hiding this comment

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

Why
Is
Every
Paraameter
On
Its
Own
Line
Humans
Dont
Read
This
Way

Copy link
Member Author

Choose a reason for hiding this comment

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

Lolz, this can be configured i think. If i remember correctly we can set max length for functions. FTR writing C in the kernel as super annoying for anyone who is OCD about uniformity when it comes to function args because each subsystem (and sometimes each file) uses its own format. While I kind of agree with you, its really hard to find a uniform format for parameters that is always right.

Copy link
Member

Choose a reason for hiding this comment

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

Kinda, you can let it run longer if you don't have many arguments, but if you end up with more arguments you really need rust-lang/rustfmt#4146 which has been open for several years now and I'm not holding my breath....That is one of the two issues I consider immediately blocking on the rust-lightning end, at least.

Copy link
Member

Choose a reason for hiding this comment

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

I really dislike the "compressed" version and prefer the "everything one its own line". Perhaps Matt does not read things in tabular form where you can clearly see the number of arguments and distinguish names from types, but I do.

self.push_opcode(opcode)
}
// We can also special-case zero
else if data == 0 {
self.push_opcode(opcodes::OP_FALSE)
}
// Otherwise encode it as data
else { self.push_scriptint(data) }
else {
self.push_scriptint(data)
Copy link
Member

Choose a reason for hiding this comment

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

Why, though? This is a really short line??

Copy link
Member Author

Choose a reason for hiding this comment

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

I lean towards thinking this one is subjective, I don't personally like the 'its a short line so format it differently' but I also agree that something like this looks retarded

if foo {
    a
} else {
    b
}

@tcharding
Copy link
Member Author

Thanks for taking the time to look at this @TheBlueMatt, it really is a kind of prickly problem because on one hand the tooling has some definite shortcomings but on the other if we don't lean into the tooling we are kind of stuck in the past and alienate those that do use the tooling. I don't have an answer, I intend on keeping exploring the problem so we can try to find an acceptable solution. Thanks again for your time.

@TheBlueMatt
Copy link
Member

Ideally rustfmt (or us, if any of us had the time lol) would add rust-lang/rustfmt#4146 and rust-lang/rustfmt#4306 then I think we could get a config that made a lot more sense. I'd still be incredibly annoyed that I can't even run rustfmt but maybe someday the rust world will care about more than just the "lulz download unverified and untrusted binaries and run those" world...though again I'm not holding my breath :(.

Ultimately the rust ecosystem turns me off in a lot of ways - I've come to terms with working on "strange" rust projects that do things differently from the "rust ecosystem" because the rust ecosystem does things in ways that are "strange" from a more classic development perspective. So be it.

@elichai
Copy link
Member

elichai commented May 11, 2022

One further concern with rustfmt - I actually can't run it! I'm not such an active contributor so its not all that important, but last I heard you can't install rustfmt unless its via rustup, which I don't think is an acceptable thing for any developer working on security-critical software to be using.

Really? it looks like it's on the distributions package manager (never tried using it through them though)
https://packages.debian.org/search?keywords=rustfmt
(and in arch the rust package provides rustc, cargo, rustfmt https://archlinux.org/packages/extra/x86_64/rust/)

@TheBlueMatt
Copy link
Member

Really? it looks like it's on the distributions package manager

Oh awesome! That is new - looks like it's not on any released Debian version only "testing", but glad to see its being addressed.

@apoelstra
Copy link
Member

I am concerned that we have an actual style disagreement here rather than just "rustfmt is stupid/inflexible". @TheBlueMatt puts a far higher premium on vertical space than I do (which surprises me given how ungodly-high resolution his laptop screen is..) My laptop screen has a 3:2 ratio, one of my external monitors is rotated 90 degrees, and I routinely have 2 or 3 terminals open in a columnar format, so I am much less concerned about this.

I also put a much higher value on visual layout. Many of the rustfmt "wasting vertical space" changes result in code where it's easier to see where things begin and end, where it's easier to count and distinguish things, etc. In particular I strongly prefer the rustfmt "all arguments are on one line, or else they're all in one column" style than a hybrid scheme where you have to explicitly read several similar-looking lines while maintaining mental counters of positioning.

@tcharding
Copy link
Member Author

Ok, this is forward progress. Stylistic differences can be resolved (arbitrarily or otherwise), issues with rustfmt are harder to resolve.

In preparation for enabling `rustfmt` crate wide add various skip
statements and do minor format fixes.

Note, for some reason adding `rustfmt::skip` to the
`sha256t_hash_newtype` causes build to fail due to ambiguous naming (we
have the exact same macro in `bitcoin_hashes`). The macros are identical
except for the `rust-bitcoin` version adds docs to the created structs.
Until that can be pushed upstream to `bitcoin_hashes` I've renamed the
macro to `_sha256t_hash_newtype` and added a comment.
@tcharding tcharding force-pushed the rustfmt branch 2 times, most recently from 2eedd19 to fe4eaaa Compare May 26, 2022 04:12
@tcharding tcharding marked this pull request as ready for review May 26, 2022 04:26
Copy link
Member

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Overall a lot cleaner than I've seen previous rustfmt code. Would kinda like to see what things look like with rust-lang/rustfmt#5337 set to compressed, but it sounds like maybe @apoelstra would disagree. The )\n.unwrap() stuff really irks me, though.

let commitment = WitnessCommitment::from_slice(
&coinbase.output[pos].script_pubkey.as_bytes()[6..38],
)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Would be really nice to get the unwrap on the same line as the ), this is exactly the kind of thing about rustfmt that pisses me off - so many lines with literally only a single ( or {.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So I think we can pretty trivially sed this one.

Copy link
Member

Choose a reason for hiding this comment

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

Like, have our own cargo format script that runs cargo fmt and then runs sed?

Neat idea but I'll bet it messes up peoples' text editor tooling.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I quite understand the issue there? Like, if you hook your text editor to run rustfmt on write you can also trivially hook your text editor to run rust-bitcoin-format.sh on write (or use @tcharding's suggestion at #959 (comment) where rustfmt wont break the existing lines, but won't put the unwrap on the right line unless the developer does so themselves, which should mean even rustfmt-on-write people won't see a lot of breakage).

Copy link
Member

Choose a reason for hiding this comment

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

The issue is Rust-specific IDEs or configuration packages where it may be difficult to replace "call rustfmt" with an arbitrary script.

Copy link
Member

Choose a reason for hiding this comment

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

Right, @tcharding's suggestion would address that concern, though. It would cause some CI failure for folks instead of local passing but...

@@ -228,7 +230,9 @@ impl From<bitcoinconsensus::Error> for Error {
}
/// Helper to encode an integer in script format
fn build_scriptint(n: i64) -> Vec<u8> {
if n == 0 { return vec![] }
if n == 0 {
return vec![];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe increase single_line_if_else_max_width? This looks like a case of that, not that its all that critical, though, really, I'm fine with this being tall.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its already set to 80 so something else is forcing this change. FTR I personally prefer the separate line here, I never liked single line early returns in C and I'm not a huge fan of them in Rust either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its got to do with the return, the following line is accepted by current rustfmt config

    let v = if n == 0 { vec![] } else { vec![] };

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, it won't let us have this either

        let rv = iter::FromIterator::from_iter(rawbytes.iter().filter_map(|&u| {
            if u > 0 { Some(u as char) } else { None }
        }));

Copy link
Member

Choose a reason for hiding this comment

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

Wow that needs better docs...

Copy link
Member

Choose a reason for hiding this comment

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

Can you open an issue on rustfmt to ask for better docs? I'd do it but I'm not sure I have exactly all the context you have, and I'm sure upstream is in a better position to update the docs than we are.

@@ -421,19 +431,29 @@ impl Script {
}

/// Returns the length in bytes of the script.
pub fn len(&self) -> usize { self.0.len() }
pub fn len(&self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe turn on fn_single_line?

Copy link
Member Author

@tcharding tcharding May 26, 2022

Choose a reason for hiding this comment

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

mmm, this is kinda hot

impl core::str::FromStr for Script {
    type Err = hex::Error;
    fn from_str(s: &str) -> Result<Self, hex::Error> { hex::FromHex::from_hex(s) }
}

I'm going to add this.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, there's def a tradeoff here...I'm open to either.

&& (sighash == EcdsaSighashType::Single
|| sighash == EcdsaSighashType::None)
{
0
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I'm not a huge fan of this being so tall either, don't see an option for it, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

This line is a mess however ya look at it :)

sequence: if n != input_index && (sighash == EcdsaSighashType::Single || sighash == EcdsaSighashType::None) { 0 } else { input.sequence },

Copy link
Member

Choose a reason for hiding this comment

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

Eh, I was really hoping for just the { 0 } else { input.sequence } being on its own line.

@tcharding
Copy link
Member Author

Thanks for putting time into this @TheBlueMatt, super cool to see you being proactive and helping us make progress even when you are defending the negative position. I'll have a play with the config options you suggest and re-spin. Cheers.

@TheBlueMatt
Copy link
Member

I'm curious what we think about #959 (comment). I find that specific case to be one of the worst offenders as far as the current rustfmt results here, and upstream gave a resounding no on accepting code to make it an option. It would be really trivial to sed the code and fix rustfmt's insanity, but then "native" rustfmt wouldn't work.

@tcharding
Copy link
Member Author

Once rust-lang/rustfmt#4306 was in I think it would be fine for devs to have to run a script to put unwrap where we want it but before that, IIUC, everytime I hit save in my editor rustfmt will put unwraps on a new line and then I'd have to run the script again on every commit after every save - that would really suck.

Add a `rustfmt` configuration file setting a bunch of configuration
options. All options that are not the default value include a comment
with the default value.

Run `cargo +nightly fmt`. No other changes made.
@TheBlueMatt
Copy link
Member

Yea, that'd make sense. Just have a script that uses an alternative rustfmt config and puts things the way we want. Though fwiw I'd imagine you can make your editor run our own script rather than rustfmt.

/// A custom key-value pair type that serialized the bytes as hex.
#[derive(Debug, Deserialize)]
struct OwnedPair<T>(
T,
#[serde(deserialize_with = "crate::serde_utils::hex_bytes::deserialize")]
Vec<u8>,
#[serde(deserialize_with = "crate::serde_utils::hex_bytes::deserialize")] Vec<u8>,
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't work out how to stop this happening?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It does read like this should control it but its currently set to 0 (default) so should not be happening?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe 0 is infinite?

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str(self.0.as_ref())
}
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.write_str(self.0.as_ref()) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Highlighting this because it might be too long for folks now? This is caused by fn_single_line = true combined with max_width = 100.

@@ -169,7 +154,8 @@ impl ::core::str::FromStr for OutPoint {
type Err = ParseOutPointError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
if s.len() > 75 { // 64 + 1 + 10
if s.len() > 75 {
// 64 + 1 + 10
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that rustfmt moves comments :/. It used to be that you could suffix lines with // to force it not to mangle them.

In any case, the original comment placement was correct and the new placement is incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem to be an option for this except to add a noformat tag here :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for flagging this. I messed with it a bunch and couldn't get a satisfactory result and yes rustfmt is clearly wrong in this. Putting the comment above line 157 is all we can do, I did mean to manually move the comment, my bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would advise to use /* smth */ next to the value (before or after, depending on the context), instead of // at the end of the line. It generally reads better, and formats better too.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f7e186d6f3d3c839a9ff59b9ff7458d1 and click "Tools -> Rustfmt"

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh it works if the comment is before the 75 but not after the 75, that is a bit silly but far better than on the newline. Thanks.

@@ -538,7 +537,8 @@ impl Transaction {
let mut input_weight = 0;
let mut inputs_with_witnesses = 0;
for input in &self.input {
input_weight += scale_factor*(32 + 4 + 4 + // outpoint (32+4) + nSequence
input_weight += scale_factor
* (32 + 4 + 4 + // outpoint (32+4) + nSequence
VarInt(input.script_sig.len() as u64).len() +
input.script_sig.len());
Copy link
Member

Choose a reason for hiding this comment

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

Here curiously rustfmt is mixing "end lines with operators" and "start lines with operators", presumably because it is confused by the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, stupid rustfmt

Copy link
Member Author

Choose a reason for hiding this comment

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

Manually changing to

            input_size += 32 + 4 + 4 // outpoint (32+4) + nSequence
                + VarInt(input.script_sig.len() as u64).len()
                + input.script_sig.len();

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this one manually, and more of the same in this file, added a separate patch for quick review. I'm going to convert this to draft while I try and make this PR more reviewable.

@apoelstra
Copy link
Member

Done reviewing 31cc094. Almost entirely unobjectionable. Left a couple of comments.

A 7000-line diff is pretty brutal to review (I think I spent 35 minutes just hitting page-down). I think if you somehow isolated the changes that purely added/removed , ; within a single line, that'd get us a large percentage of the diff which would be unobjectionable and quickly mergeable. Though I dunno what kind of script magic you'd need to do for that..

Similarly with collapsing multiline functions to one line, and reordering import statements.

@tcharding
Copy link
Member Author

Done reviewing 31cc094. Almost entirely unobjectionable. Left a couple of comments.

Thanks for reviewing!

A 7000-line diff is pretty brutal to review (I think I spent 35 minutes just hitting page-down).

Bother, sorry to take so much of your time.

I think if you somehow isolated the changes that purely added/removed , ; within a single line, that'd get us a large percentage of the diff which would be unobjectionable and quickly mergeable. Though I dunno what kind of script magic you'd need to do for that..

Challenge accepted :)

In order to make rustfmt leave the comment on the same line as the magic
number we have to put it _before_ the number.
In order to get the rustfmt to play nicely with multiline math
statements that include comments put the operator at the start of the
line.
@tcharding tcharding marked this pull request as draft May 30, 2022 23:50
@tcharding
Copy link
Member Author

Challenge accepted :)

And failed. I came up with this method of introducing rustfmt though: #1040

@tcharding
Copy link
Member Author

Closing this in favour of #1040

@tcharding tcharding closed this Jun 24, 2022
@tcharding tcharding deleted the rustfmt branch June 29, 2022 02:24
sanket1729 added a commit that referenced this pull request Jul 19, 2022
ee9a3ec Enable formatter for "src" (Tobin C. Harding)
743b197 Add use for Unexpected (Tobin C. Harding)
fd217e7 Use f instead of formatter (Tobin C. Harding)
7b50a96 Do not format prelude module (Tobin C. Harding)
01471f7 Shield hash_newtypes from rustfmt (Tobin C. Harding)
65d19d9 Refactor compile_error message (Tobin C. Harding)
6461e2d Run formatter on examples/ (Tobin C. Harding)
fd1c658 Add a rustfmt configuration file with explicit ignores (Tobin C. Harding)

Pull request description:

  Looks like we are getting to a place that rustfmt _might_ be able to be used. In an effort to introduce `rustfmt` without requiring devs to do excessively mundane reviews introduce `rustfmt` in a non-invasive manner. What this means is

  - Add a fully fleshed out `rustfmt` config file that explicitly ignores all the source files
  - Enable sections of code one by one so review is easier (including preparatory patches before doing each section).

  This PR currently does `examples` and all the source files at the root level of the crate (i.e. excludes all directories in `src/`). The other directories can then be done one at a time.

  Please see discussion on: #959 for more context.

ACKs for top commit:
  sanket1729:
    ACK ee9a3ec
  apoelstra:
    ACK ee9a3ec

Tree-SHA512: f4ff3c031b5d685777e09bac2df59ed8217576b807da7699f44fe00aa509d0b7fe47617a8b3eff00d3125aeece3e010b8f9dd8733d315ccb2adc0e9a7d7f07e3
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
…sive manner

ee9a3ec Enable formatter for "src" (Tobin C. Harding)
743b197 Add use for Unexpected (Tobin C. Harding)
fd217e7 Use f instead of formatter (Tobin C. Harding)
7b50a96 Do not format prelude module (Tobin C. Harding)
01471f7 Shield hash_newtypes from rustfmt (Tobin C. Harding)
65d19d9 Refactor compile_error message (Tobin C. Harding)
6461e2d Run formatter on examples/ (Tobin C. Harding)
fd1c658 Add a rustfmt configuration file with explicit ignores (Tobin C. Harding)

Pull request description:

  Looks like we are getting to a place that rustfmt _might_ be able to be used. In an effort to introduce `rustfmt` without requiring devs to do excessively mundane reviews introduce `rustfmt` in a non-invasive manner. What this means is

  - Add a fully fleshed out `rustfmt` config file that explicitly ignores all the source files
  - Enable sections of code one by one so review is easier (including preparatory patches before doing each section).

  This PR currently does `examples` and all the source files at the root level of the crate (i.e. excludes all directories in `src/`). The other directories can then be done one at a time.

  Please see discussion on: rust-bitcoin/rust-bitcoin#959 for more context.

ACKs for top commit:
  sanket1729:
    ACK ee9a3ec
  apoelstra:
    ACK ee9a3ec

Tree-SHA512: f4ff3c031b5d685777e09bac2df59ed8217576b807da7699f44fe00aa509d0b7fe47617a8b3eff00d3125aeece3e010b8f9dd8733d315ccb2adc0e9a7d7f07e3
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

5 participants