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

One shot cargo fmt #80

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sanket1729
Copy link
Member

@sanket1729 sanket1729 commented Apr 19, 2021

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.

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,
Copy link
Member

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

Copy link
Collaborator

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?

@apoelstra
Copy link
Member

Yeah, I think I can live with this

@stevenroose
Copy link
Member

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 :|

@sanket1729
Copy link
Member Author

@RCasatta I am still seeking concept ACKs on this. What are your opinions on cargo fmt?

@RCasatta
Copy link
Collaborator

@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.

@sanket1729 sanket1729 reopened this Dec 16, 2021
if [ "$DO_LINT" = true ]
then
(
rustup component add rustfmt
Copy link
Collaborator

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

@sanket1729
Copy link
Member Author

I plan to go through the diff and add another commit adding rustfmt::skip.

@sanket1729
Copy link
Member Author

Based on
Yeah, I think I can live with this - @apoelstra
cargo fmt is a strong concept ACK for me - @RCasatta

and a minor complaint
I'm generally not a fan of formatters. ...won't veto anything ofc - @stevenroose

I will open this for review after adding rustfmt::skip to oddly formatted things.

@delta1
Copy link
Member

delta1 commented May 19, 2022

concept ACK - I don't ever want to think or bikeshed about formatting

@sanket1729
Copy link
Member Author

Reviewing this with a refreshed ACK from @delta1 and the comments posted before. #80 (comment)

@sanket1729 sanket1729 mentioned this pull request Oct 1, 2022
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.

5 participants