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

feat(fmt): add sort_imports config #5442

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

meetmangukiya
Copy link
Contributor

@meetmangukiya meetmangukiya commented Jul 20, 2023

Motivation

Sorting imports is a desirable feature.
Closes #3396. I didn't check the issues first and went head first into the implementation because I wanted import sorts but see now that @rkrasiuk is already assigned to the issue 😅 .

Solution

Sorts the SourceUnitPart::Import parts before running it through the formatter. Sort works but blanks and slicing need to be fixed. Also, as I built this I have realized few things:

  1. Contiguous imports detection based off just contiguous SourceUnitPart::Import wont be correct because blank lines do not have a SourceUnit entry. Need to use self.blank_lines somewhere to better determine the slice boundaries.
  2. Currently, it uses blank_lines to determine if there should be a space between formatted units, need to fix it. I am thinking maybe there is a way to use the needs_space callback in conjunction with the import_slices to determine the blank spaces to add correctly.

Opening a WIP PR, because it might not be the best or even a correct way to do this.

@Evalir
Copy link
Member

Evalir commented Aug 15, 2023

hey @meetmangukiya i think this is good! do you plan on finishing this, or should we take over?

@meetmangukiya
Copy link
Contributor Author

Will get back to this when I find time. In the mean time if you want to take over it's also fine.

@meetmangukiya
Copy link
Contributor Author

meetmangukiya commented Aug 18, 2023

I also am interested in a feature for sorting functions by receive, fallback, visibility, mutability, payable, etc. as here. Which I think could be done similarly? Maybe could reuse some parts from here too.

@aviggiano
Copy link

Hello

This is a great feature.
Would it be possible to also include "remove unused imports" in the PR?

@mattsse
Copy link
Member

mattsse commented Dec 11, 2023

I completed this by taking the original approach and merely modifying the group detection.

This currently only sorts individual import groups (separated by newline) and does not reorder groups. I guess we could reorder groups as well but this should be a separate setting.

there's a limitation when a group contains a comment, but we can live with that for now.
perhaps there are other edge cases but since this is opt-in, this should be good for now until they get reported.

@meetmangukiya
Copy link
Contributor Author

🐐

@mattsse mattsse changed the title sort imports wip feat(fmt): add sort_imports config Dec 11, 2023
@Evalir Evalir merged commit cdbaf9d into foundry-rs:master Dec 11, 2023
19 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.

feat (fmt): sort imports
4 participants