Sort imports #292

2020-04-06T19:26:08+02:00

lspitzner commented

2020-04-06 19:26:08 +02:00

(Migrated from github.com)

This implements the basic functionality, including some trickery to handle comments sensibly.

Terminology:

import Foo ( MyData(MyExposedConstructor), myFunction )
                    \----constructor----/
             \----------item------------/  \--item--/
\----------------------import-------------------------/

import A                       \ import block
-- comment on why we import B  |
import qualified B             |
import C                       /

-- comment in the middle       | independent comment (not connected to import D)

import D ( a                   \ import block
         , b                   |
         , b                   |
         )                     | 
import E                       /

-- that is, blocks are series of import statements that don't contain any empty lines.

-- Merging (for example on constructors):
import A ( MyType(Foo), MyType(Bar) )
-- -->
import A ( MyType (Foo, Bar) )

-- question left for the reader: How can these be merged?
import qualified A (foo, bar)
import           A hiding (bar, baz)

Implemented features

Sort import statements
Sort and uniq items
Sort and uniq constructors
Merge constructors
Respect blocks of imports, only sort inside blocks
Sorting imports moves comments connected to those imports around, but keeps all comments
Retain newlines between comments and keep independent comments unmodified

Issues that must be fixed before merging

I messed up and CI says the code does not compile. Whoops.
Fix existing tests
Fix compat with different ghc versions
Needs some new tests
Should be tested on some larger existing code-bases

all other are non-blockers

Features yet to be implemented

Merge imports
Sort exports - basics would work already, even module exports work already or are trivial, but haddock headers and sections are really annoying to get right in general.

Paragraph-fill version of item lists

import Foo
  ( longNameOne, longNameTwo, longNameThree, longNameFour, longNameFive
  , longNameSix, longNameSeven, longNameEight, longNameNine, longNameTen
  , longNameEleven, longNameTwelve, longNameThirteen, longNameFourteen
  , longNameFifteen
  )

Issues to be fixed, low priority

Comments on items and on constructors get lost when merging happens

See #155 This implements the basic functionality, including some trickery to handle comments sensibly. ### Terminology: ~~~~ import Foo ( MyData(MyExposedConstructor), myFunction ) \----constructor----/ \----------item------------/ \--item--/ \----------------------import-------------------------/ import A \ import block -- comment on why we import B | import qualified B | import C / -- comment in the middle | independent comment (not connected to import D) import D ( a \ import block , b | , b | ) | import E / -- that is, blocks are series of import statements that don't contain any empty lines. -- Merging (for example on constructors): import A ( MyType(Foo), MyType(Bar) ) -- --> import A ( MyType (Foo, Bar) ) -- question left for the reader: How can these be merged? import qualified A (foo, bar) import A hiding (bar, baz) ~~~~ ### Implemented features - [x] Sort import statements - [x] Sort and uniq items - [x] Sort and uniq constructors - [x] Merge constructors - [x] Respect blocks of imports, only sort inside blocks - [x] Sorting imports moves comments connected to those imports around, but keeps all comments - [x] Retain newlines between comments and keep independent comments unmodified ### Issues that must be fixed before merging - [x] I messed up and CI says the code does not compile. Whoops. - [x] Fix existing tests - [x] Fix compat with different ghc versions - [ ] Needs some new tests - [ ] Should be tested on some larger existing code-bases all other are **non-blockers** ### Features yet to be implemented - [ ] Merge imports - [ ] Sort exports - basics would work already, even module exports work already or are trivial, but haddock headers and sections are really annoying to get right in general. - [ ] Paragraph-fill version of item lists ~~~~ import Foo ( longNameOne, longNameTwo, longNameThree, longNameFour, longNameFive , longNameSix, longNameSeven, longNameEight, longNameNine, longNameTen , longNameEleven, longNameTwelve, longNameThirteen, longNameFourteen , longNameFifteen ) ~~~~ ### Issues to be fixed, low priority - [ ] Comments on items and on constructors get lost when merging happens

👍 1

expipiplus1 commented

2020-04-10 10:12:01 +02:00

(Migrated from github.com)

@lspitzner I've got a large codebase which I've been deferring neatening imports on until brittany had support! I'll give this branch a test run this weekend.

expipiplus1 commented

2020-04-27 15:00:02 +02:00

(Migrated from github.com)

@lspitzner works very well! dcca2969c3

I like how it preserves groups of imports and doesn't barf on {-# SOURCE #-} imports.

If possible: when you live code next could it be friendly to Singapore time?

@lspitzner works very well! https://github.com/expipiplus1/vulkan/commit/dcca2969c301c0a062c489150983a5f767f336f8 I like how it preserves groups of imports and doesn't barf on `{-# SOURCE #-}` imports. If possible: when you live code next could it be friendly to Singapore time?

tfausak commented

2020-04-27 16:02:30 +02:00

(Migrated from github.com)

I don't have a public codebase to point to, but this worked great on our ~80 KLOC project 👍

I don't have a public codebase to point to, but this worked great on our ~80 KLOC project :+1:

expipiplus1 commented

2020-06-02 08:48:21 +02:00

(Migrated from github.com)

@lspitzner This is a great improvement already, would it be possible to get this merged and move the exising TODO items to issues?

lspitzner commented

2020-06-02 22:24:38 +02:00

(Migrated from github.com)

sorry I am so unresponsive atm. I have been / am still struggling to move all my maintenance and potentially CI over to nix. It has been a bit frustrating but I think it will be worth it in the long run.

There are two minor fixups I wanted to add on this branch, both simple "if this special case occurs, just fall back on not modifying the thing in question". These should indeed make this a useful addition without any downsides even for users that might have curious comments on their imports. Any other improvements can indeed be moved to separate issues.

So I am not taking a summer break, but the priorities will be nix > ghc810 > sorting imports.

sorry I am so unresponsive atm. I have been / am still struggling to move all my maintenance and potentially CI over to nix. It has been a _bit_ frustrating but I think it will be worth it in the long run. There are two minor fixups I wanted to add on this branch, both simple "if this special case occurs, just fall back on not modifying the thing in question". These should indeed make this a useful addition without any downsides even for users that might have curious comments on their imports. Any other improvements can indeed be moved to separate issues. So I am not taking a summer break, but the priorities will be nix > ghc810 > sorting imports.

👍 1

expipiplus1 commented

2020-06-17 16:03:21 +02:00

(Migrated from github.com)

@lspitzner Is there any specific help you could use on the nix side of things? FWIW this very simple github action (using cachix for caching) has been working well for me: https://github.com/expipiplus1/vulkan/blob/master/.github/workflows/ci.yml#L170-L186

expipiplus1 commented

2020-11-12 14:40:01 +01:00

(Migrated from github.com)

I've been using this for quite some time, and think it's certainly stable enough to be merged as is, perhaps conditional on a config setting as it's considered incomplete.

tfausak commented

2020-12-11 14:09:53 +01:00

(Migrated from github.com)

Closing this in favor of #330.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#292