nwolverson / purescript-language-server

MIT License
184 stars 41 forks source link

Update `Import` commands #123

Closed vanceism7 closed 3 years ago

vanceism7 commented 3 years ago

So here's the accompanying PR to the other I made in the vscode ide repo.

This PR basically takes advantage of that extra data being passed in and uses that to invoke addCompletionImportEdit to handle the importing. This fixes normal and qualified imports

Currently, invoking Add Import would just leave an open import which isn't really what you'd expect.

E.g: If I tried to invoke import on either of the following lines

foo = maybe

foo2 = Qualified.Maybe.maybe

I'd end up with the following at the top

import Data.Maybe

With this PR, now the first line will import explicit:

import Data.Maybe (maybe)

And the 2nd line will import qualified:

import Data.Maybe as Qualified.Maybe

I realize I may be way out on a whim here and actually don't understand the deeper implications of what this does, so if this is just plain wrong, I can fix right away!

Thanks for taking the time!

nwolverson commented 3 years ago

Currently, invoking Add Import would just leave an open import which isn't really what you'd expect.

I'm sorry, but this is exactly the expectation, at least that's what the command has always done - there is "Add Import" and "Add Explicit Import", the implication being that the former is not explicit.

Now in the case of qualifiers - definitely there is nothing there for that case (well, apart from completion which is what I actually use instead of these commands). And yes rather than 2 more commands to fill out the explicit/qualified truth table maybe doing it with "magic" is a good idea - only thing is it's not possible to explicitly select that (in vscode) without starting from the cursor on an existing identifier - which is probably fine.

vanceism7 commented 3 years ago

I'm sorry, but this is exactly the expectation, at least that's what the command has always done

Sorry I should've said, this isn't what you'd expect compared to the equivalent psc-ide command. In psc-ide, calling the import command uses the text from the cursor position to guide the import. If there's no qualifier, it does an explicit import; if there is a qualifier, it does a qualified import. I don't believe psc-ide supplies an "add explicit import" command, although I could be wrong since I haven't used emacs for a bit of time now.

The advantage of importing via the import command over the auto-completion is that you get the option to do a text search for the module you want to use. In the auto-completion version, you are just given a list of possible functions and have to manually up-down arrow through the menu until you find the module you're looking for.

Perhaps this idea of automatically choosing between qualified or explicit import would better belong in the "Add Explicit Import" command? Or maybe the better idea is to just add the qualified import functionality to the "add import" command and leave explicit imports to the "add explicit import" command?

nwolverson commented 3 years ago

My initial preference (given that we're starting from existing functionality) would be to retain the distinction of the current commands, but have them both respect the qualification of the identifier.

So

All 4 cases are useful to some extent:

1a. Open import - not encouraged in general (ie compiler warning), but often useful during development, 1b. Qualified import - commonly used 2a. Explicit import - commonly used 2b. Qualified explicit import - used when importing multiple related modules qualified (if they were not explicit, you would get a compiler warning). Notably I don't think there's a way to add a new module to that set with auto-complete

I can probably come around to the idea of scrapping the distinction and having 1 command that is more opinionated. Or possibly 1 such command for explicit/qualified import and rename "Add Import" to "Add Open Import" if there's value in still having it.

vanceism7 commented 3 years ago

I think ideally the last option of renaming "Add Import" to "Add Open Import" and then having an opinionated "Add Import" would be my preference. I don't think it would be too much more work from the current state of this PR to implement either.

It's up to you though! I'm ok if you'd prefer to go with a more incremental approach.

Thanks again for taking the time to discuss

nwolverson commented 3 years ago

Let's go with that option then, in a quick poll at least 1 other person agrees.

vanceism7 commented 3 years ago

@nwolverson hey how's this PR looking? I think we got everything in here that we need per our discussion

nwolverson commented 3 years ago

Thanks for the update - I think I need to get a release out addressing 0.14 changes, I want to test this out and merge but not sure if that might be after that release.

vanceism7 commented 3 years ago

Ah, didn't realize 0.14 was just released, very cool! Anyways thanks, let me know if there's anything else I need to do here