Delete unused top level binding code action #657
Conversation
|
|
serhiip
force-pushed
the
code-action-delete-binding
branch
from
72a5a72 to
9685ac3
Compare
| "{-# OPTIONS_GHC -Wunused-top-binds #-}" | ||
| , "module A (some) where" | ||
| , "" | ||
| , "" |
There was a problem hiding this comment.
A very minor remark, but beforehand they had 1-line gaps, now they have 2-line gaps. Should we expand one line after if it's blank? Or maybe this is getting ahead of ourselves and there is no need.
There was a problem hiding this comment.
Makes sense to me. What do you think @serhiip?
There was a problem hiding this comment.
For sure expanding the text edit until the next binding in a file is a good thing. Now the code works by finding the following binding after the current one and expands the deletion until that subsequent binding (if any). To make that happen, I rearranged my code a bit. I hope it makes more sense now. Thanks for your suggestions
There was a problem hiding this comment.
Great work, thanks @serhiip! Two small comments. I would suggest to not tackle operators in that PR, we can try to tackle them separately.
| = [("Delete ‘" <> name <> "’", [TextEdit (nextLine . srcSpanToRange $ l) "" | (L l _) <- sameName])] | ||
| | otherwise = [] | ||
| where | ||
| isTopLevel l = (_character . _start) (srcSpanToRange l) == 0 |
There was a problem hiding this comment.
You are basically duplicating this logic in line 188. How about we change this to accept a range and then we can use it for both cases (composing with srcSpanToRange at one call site).
There was a problem hiding this comment.
Indeed! There was a bug with infix bindings, so I've eradicated line 188 completely to fix that bug. I also made srcSpanToRange called only once per binding location
| "{-# OPTIONS_GHC -Wunused-top-binds #-}" | ||
| , "module A (some) where" | ||
| , "" | ||
| , "" |
There was a problem hiding this comment.
Makes sense to me. What do you think @serhiip?
- handle case of top level bindings defined in infix form
- when deleting some unused binding expand text deletion range to
beginning of next top level binding (if any was found)
Good point. And actually there was a bug :). Luckily it was easy to fix. I pushed the fixed version and added corresponding test (just in case). It was a one line fix so I've incorporated it together with other changes I did. |
| forwardLines lines r = r {_end = (_end r) {_line = (_line . _end $ r) + lines, _character = 0}} | ||
|
|
||
| toNextBinding bindings r@Range { _end = Position {_line = l} } | ||
| | Just (Range { _start = Position {_line = l'}}, _) <- find ((> l) . _line . _start . fst) bindings |
There was a problem hiding this comment.
Is hsmodDecls guaranteed to be sorted by ranges? Looking at haddock’s collectDocs it always seems to be called after sortLocated suggesting that we probably need this here as well.
There was a problem hiding this comment.
Order in hsmodDecls doesn't matter really - I use find to locate next binding within all the top level bindings. I might have missed the point why hsmodDecls must be sorted
There was a problem hiding this comment.
find will give you the first match. where the condition is true, consider what happens if you have 3 bindings. You want to delete the first. Assume hsModDecls has the order 1, 3, 2. Then your find here will find 3 and you will end up deleting 2 as well
There was a problem hiding this comment.
My bad. That is indeed an issue - I've pushed an update. Thanks for catching this
Sort all inspected bindings by location before processing
serhiip
force-pushed
the
code-action-delete-binding
branch
from
56a4740 to
da17a08
Compare
Happens when there is unused local binding
serhiip
force-pushed
the
code-action-delete-binding
branch
from
d50a9b0 to
3465d66
Compare
* Delete unused top level binding code action
* Remove redundant brackets according to hlint
* Attempt to fix build issue on ghc-8.4
* Fix delete unused binding code action
- handle case of top level bindings defined in infix form
- when deleting some unused binding expand text deletion range to
beginning of next top level binding (if any was found)
* Modify delete unused binding code action
Sort all inspected bindings by location before processing
* Avoid sending top level binding delete action with no TextEdit
Happens when there is unused local binding
* Delete unused top level binding code action
* Remove redundant brackets according to hlint
* Attempt to fix build issue on ghc-8.4
* Fix delete unused binding code action
- handle case of top level bindings defined in infix form
- when deleting some unused binding expand text deletion range to
beginning of next top level binding (if any was found)
* Modify delete unused binding code action
Sort all inspected bindings by location before processing
* Avoid sending top level binding delete action with no TextEdit
Happens when there is unused local binding
* Delete unused top level binding code action
* Remove redundant brackets according to hlint
* Attempt to fix build issue on ghc-8.4
* Fix delete unused binding code action
- handle case of top level bindings defined in infix form
- when deleting some unused binding expand text deletion range to
beginning of next top level binding (if any was found)
* Modify delete unused binding code action
Sort all inspected bindings by location before processing
* Avoid sending top level binding delete action with no TextEdit
Happens when there is unused local binding
This is my first contribution. My goal is contribute to haskell-language-server / ghcide constantly. This adds a code action that deletes unused top level binding.
Please, let me know how this should be improved.
This closes #54 (I think)