-
Notifications
You must be signed in to change notification settings - Fork 33
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
One shot cargo fmt #80
base: master
Are you sure you want to change the base?
Conversation
src/transaction.rs
Outdated
0xde, 0x24, 0xfd, 0xac, 0x6e, 0x9f, 0x1f, 0xae, | ||
0x02, 0xe2, 0x5e, 0x58, 0x2a, 0xc1, 0xad, 0xc6, 0x9f, 0x16, 0x8a, 0xa7, | ||
0xdb, 0xf0, 0xa9, 0x73, 0x41, 0x42, 0x1e, 0x10, 0xb2, 0x2c, 0x65, 0x99, | ||
0x27, 0xde, 0x24, 0xfd, 0xac, 0x6e, 0x9f, 0x1f, 0xae, |
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.
this kinda thing really really bugs me about rustfmt
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.
#[rustfmt::skip] on this bytes vec fields?
Yeah, I think I can live with this |
I'm generally not a fan of formatters. I think formatting can help in presenting and conveying information so formatting is part of the process of writing code and formatting can/should also be a subject of review. Readability of code is important, especially in an information-dense language like Rust. Anyway, my 2 cents, won't veto anything ofc. But just went through the struggle of formatting rust-secp256k1-zkp various times after submitting to CI :| |
@RCasatta I am still seeking concept ACKs on this. What are your opinions on cargo fmt? |
cargo fmt is a strong concept ACK for me. Sometimes it's not as perfect as manual formatting, like not having bytes vec in octets or spanning a method chain in million lines. However, the benefit of having an automatic check in CI and less formatting nits turnaround outweighs the downsides. Also, I personally find it useful to write code without thinking about formatting cause I know you can fix it easily and without a doubt. |
if [ "$DO_LINT" = true ] | ||
then | ||
( | ||
rustup component add rustfmt |
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.
it looks it is already installed, CI prints:
info: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is up to date
I plan to go through the diff and add another commit adding |
Based on and a minor complaint I will open this for review after adding |
concept ACK - I don't ever want to think or bikeshed about formatting |
Reviewing this with a refreshed ACK from @delta1 and the comments posted before. #80 (comment) |
I think we should do it earlier when the diff is smaller and the project is smaller in scope. Otherwise, it becomes a big task to do it file by file as with rust-bitcoin. So far, we have had good experiences with rust-miniscript. Most rust-bitcoin developers are also in favor of using it, and I think we should get it done when the diff is relatively small and very easy to review/reproduce.
Curious what do @apoelstra, @stevenroose, and others think about this.