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

fmt: add group_imports = StdExternalCrate #9141

Closed
wants to merge 2 commits into from

Conversation

tcoratger
Copy link
Contributor

Related #9041 (comment)

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

supportive,

we have a few larger prs outstanding where this would cause friction, so I'm delaying this for a bit.

could we just do the additional lint and then we format and merge when ready?
perhaps even close and resubmit this if easier

@tcoratger
Copy link
Contributor Author

supportive,

we have a few larger prs outstanding where this would cause friction, so I'm delaying this for a bit.

could we just do the additional lint and then we format and merge when ready? perhaps even close and resubmit this if easier

@mattsse No worries, let's do it like last time on the clippy. Tell me as soon as you're ready to merge, and I'll update this PR afterwards, leave it open, I'll deal with it :)

pub use reth_execution_errors::{BlockExecutionError, BlockValidationError};
use reth_execution_types::ExecutionOutcome;
use reth_primitives::{BlockNumber, BlockWithSenders, Receipt, Request, U256};
use reth_prune_types::PruneModes;
pub use reth_storage_errors::provider::ProviderError;
use revm::db::BundleState;
use revm_primitives::db::Database;
Copy link
Member

Choose a reason for hiding this comment

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

as suggested, would have been good if an issue was opened to discuss this before the work was put in to impl this. we don't want this lint because it doesn't make a difference between pub use and private use statements.

if you can revert the mixing of pub use and private use statements, and remove the lint from ci, we can merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is possible here:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't hesitate to close if needed this is not a problem, this was just adding one line to the fmt file and running cargo +nightly fmt that's why I didn't open an issue for this before implementation.

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 fix it manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I tried but as soon as we provide a group_import element in the fmt, it seems that all imports are automatically rearranged at save...

@tcoratger tcoratger reopened this Jun 28, 2024
@emhane
Copy link
Member

emhane commented Jun 29, 2024

closing until group_by_visibility flag is stable

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.

3 participants