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

style: enable fmt linting for grouped imports by std external and crates #1603

Merged
merged 3 commits into from
Dec 8, 2024

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Dec 8, 2024

What was wrong?

We don't have the fmt linting option for the import format we all use.

The format we normally use as a team is

-1 std imports grouped up
-2 external imports grouped up
-3 crates imports grouped up

How was it fixed?

It turns out fmt has a flag for enabling this formatting so I enabled the lint option in rustfmt.toml

@KolbyML KolbyML self-assigned this Dec 8, 2024
@KolbyML KolbyML changed the title style: enable linting for grouped imports style: enable linting for grouped imports by std external and crates Dec 8, 2024
@KolbyML KolbyML changed the title style: enable linting for grouped imports by std external and crates style: enable fmt linting for grouped imports by std external and crates Dec 8, 2024
Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

Rubber stamp approval (checked dozens of files and they look fine).

:shipit:


Sidenote: Our trin book page says that groups should be:

  • Imports from 'std'
  • Imports from external crates
  • Imports from crates within trin

I think this can be interpreted that other trin crates should be imported inside 3rd group, something like this:

// e.g. inside trin-storage/src
use alloy::{...};

use crate::{..};
use ethportal_api::{..};

But in reality, ethportal_api would be considered external crate, and be part of the 2nd group.

I think that we can't setup linter to do the above, so maybe while you are at it, can you update docs page to clarify.

@KolbyML KolbyML merged commit 38b41c2 into ethereum:master Dec 8, 2024
12 checks passed
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.

2 participants