-
Notifications
You must be signed in to change notification settings - Fork 206
Conversation
e78865f
to
c035e47
Compare
Due to unexpected changes in #1167, this patch does not work when rebased. For some reason it duplicates the import line. You can try it now, but i have to take a long look at it again. Will do that tomorrow, as I think this feature is great. |
c035e47
to
7ef6e4b
Compare
2265c33
to
6ad3a2d
Compare
Now it works, but with a quirk: the Brittany formatter adds a newline when repeatedly used to import a single function. |
2120fd0
to
fe8ed0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
liftIO $ runBrittany tabSize confFile text | ||
where tabSize = opts ^. J.tabSize | ||
|
||
-- | Extend to the line below to replace newline character, as above. | ||
-- | Extend to the line below and above to replace newline character. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this extend the range to the line above? It looks like it does
xxxSooo
ooExxxx
xxx
=>
Soooooo
oooooo
Exxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this looks quite handy. Would it be worthwhile look at moving these helper functions into haskell-lsp-types
? cc @alanz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be handy to do that, but as a follow-up, this PR has had a very long history already, its unfair to ask for that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is out of scope for this PR. I think it would make more sense as part of a larger refactoring
= Exact -- ^ If you want to match exactly the search string. | ||
| ExactName -- ^ If you want to match exactly a function name. | ||
-- Same as @Exact@ if the term is just a function name. | ||
| Relax (T.Text -> T.Text) -- ^ Relax the search term to match even more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be sensible to move this to the Hoogle plugin. But right now, nobody else uses it, so it stays in HsImport.
-- A formatting type can be given to either format the whole document or only a Range. | ||
-- | ||
-- Text to format, may or may not, originate from the associated Uri. | ||
-- Should, but might not, honour the provided formatting options (e.g. Floskell does not). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: Should we use US or British English? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it really matters anymore, tbh. But my bias is toward what I know, which is British style.
c51e16e
to
55b2315
Compare
3ebf677
to
e9e695b
Compare
Adds a parameterized test suite for formatter providers. Should be used when introducing new formatters.
Needs to be fixed.
381acb7
to
85ac6e4
Compare
The feature is implemented and was manually tested. |
CI fails are imo unrelated to this PR. |
Got the tests working on my machine, took a bit of fiddling, fair play for getting this far without being able to run them! This PR has been waiting for a while, @alanz is this good to merge once CI passes? |
Did this get fixed, @fendor? |
@expipiplus1, yes that has been fixed! @bubba, that is great, thank you very much! |
134b1c5
to
049609d
Compare
This doesn't seem to work for me, I'm using 0.10.0.0, and the only code actions I have available are the usual ones like duplicate definition & co. Is there anything needed to get this to work? |
Hold on I'll debug some more, it seems to work in one project but not the other. |
It mainly works for values, not types. E.g. try to use |
Yeah I'm using the very example from this PR to test. I'll be back once I have a minimal reproducible example |
So it happens even with the simplest new project: https://github.com/Infinisil/hsimport-repro I just run This is with a nixpkgs whose default GHC is 8.6.5, but I also tested with 8.6.4 and 8.4.4, and it's all the same. Here's the logs:
I mentioned that it did work for one project, well now it doesn't anymore, and I'm not sure what changed between then (the project itself stayed the same for sure). What is also a bit weird is that it doesn't seem to show all lines of the diagnostics which it should do in 0.10.0.0 (assuming there's supposed to be more than just "Variable not in scope"), however that does work in another project, so ehh. |
This is probably worth its own issue, thank you for reporting in such detail! |
As figured out by @fendor on IRC, this is most likely because I didn't have a Hoogle database. I'll try again later with one, and will investigate if I could ship a database with |
@infinisil any news on shipping a Hoogle database with |
While I think it would be possible to wrap the hie executables with Ideally HIE would use what I described in #128 |
Closes #1086
Offers code action to add a function to import list.
E.g.
With 2 code actions this can be turned into:
Requires: