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

chore: introduce rustfmt.toml #5686

Closed
wants to merge 1 commit into from
Closed

chore: introduce rustfmt.toml #5686

wants to merge 1 commit into from

Conversation

kamuik16
Copy link
Contributor

Description

closes #5684

Uses cargo +nightly fmt for

  • reordering and formatting imports for better formatting and readability
  • formatting code in the /// doc comments

Copy link
Contributor

mergify bot commented Nov 25, 2024

This pull request has merge conflicts. Could you please resolve them @kamuik16? 🙏

@drHuangMHT
Copy link
Contributor

I think the diffs are too large, and some make more sense than others. Also I am not really a fan of so many line breaks.

@kamuik16
Copy link
Contributor Author

I think the diffs are too large, and some make more sense than others. Also I am not really a fan of so many line breaks.

okay. thou I don't know what you mean by line breaks?

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks! Overall looks very good to me, left just a couple comments

@@ -1,3 +1,6 @@
- Introduce `rustfmt.toml`.
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to add a CHANGELOG.md entry for this

Copy link
Contributor Author

@kamuik16 kamuik16 Nov 26, 2024

Choose a reason for hiding this comment

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

we don't need to add a CHANGELOG.md entry for this

if I don't add an entry in the CHANGELOG.md entry for this, the CI will be all red because it thinks the diffs are protocol changes. I am okay, if that is something you are okay with.

Copy link
Member

Choose a reason for hiding this comment

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

that's because you titled it as feat, if you change to chore it won't

Multiaddr,
};
use either::Either;
use futures::prelude::*;
use pin_project::pin_project;
use std::{pin::Pin, task::Context, task::Poll};
Copy link
Member

Choose a reason for hiding this comment

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

can we not break here, and therefore not diff on these situations? I think it's what @drHuangMHT was mentioning also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we not break here, and therefore not diff on these situations? I think it's what @drHuangMHT was mentioning also

I don't think I can control the output of the diff here, but I don't see huge downside of this. lmk wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just cargo +nightly fmt on the codebase after adding the rustfmt.toml.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's the imports_layout rule, can you test with LimitedHorizontalVertical? see here and here for more info

@@ -37,7 +37,8 @@ mod proto {
#![allow(unreachable_pub)]
include!("generated/mod.rs");
pub use self::{
envelope_proto::*, peer_record_proto::mod_PeerRecord::*, peer_record_proto::PeerRecord,
envelope_proto::*,
Copy link
Member

Choose a reason for hiding this comment

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

since we are on this, is there a way to impede * imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we are on this, is there a way to impede * imports?

aah no, not using the rustfmt as far as I know. do correct me if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

however there's a clippy lint config warn-on-all-wildcard-imports which we can set to true in the Cargo.toml and it will give us the warnings about these. we'd then have to fix them manually. I'd consider that a seperate issue for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

ok nice, yeah we can submit another PR with it enabled

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks for your response, left a couple suggestions in the rustfmt.toml file can you run with those added?

@@ -1,3 +1,6 @@
- Introduce `rustfmt.toml`.
Copy link
Member

Choose a reason for hiding this comment

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

that's because you titled it as feat, if you change to chore it won't

@@ -0,0 +1,3 @@
reorder_imports = true
imports_granularity = "Crate"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
imports_granularity = "Crate"
imports_granularity = "Crate"
group_imports = "StdExternalCrate"

@@ -0,0 +1,3 @@
reorder_imports = true
imports_granularity = "Crate"
format_code_in_doc_comments = true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
format_code_in_doc_comments = true
format_code_in_doc_comments = true
format_strings = true
normalize_comments = true
wrap_comments = true

@@ -37,7 +37,8 @@ mod proto {
#![allow(unreachable_pub)]
include!("generated/mod.rs");
pub use self::{
envelope_proto::*, peer_record_proto::mod_PeerRecord::*, peer_record_proto::PeerRecord,
envelope_proto::*,
Copy link
Member

Choose a reason for hiding this comment

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

ok nice, yeah we can submit another PR with it enabled

Multiaddr,
};
use either::Either;
use futures::prelude::*;
use pin_project::pin_project;
use std::{pin::Pin, task::Context, task::Poll};
Copy link
Member

Choose a reason for hiding this comment

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

I think it's the imports_layout rule, can you test with LimitedHorizontalVertical? see here and here for more info

@kamuik16 kamuik16 changed the title feat: introduces rustfmt.toml chore: introduce rustfmt.toml Nov 27, 2024
@kamuik16
Copy link
Contributor Author

Had some trouble with git, so had to close this one for #5694 :)

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.

Code formatting with rustfmt.toml config
3 participants