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

HLS does not suggest Data.ByteString.Internal for possible imports #3369

Open
santiweight opened this issue Nov 28, 2022 · 9 comments
Open
Labels
status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@santiweight
Copy link
Collaborator

GHC 9.2.4
HLS 1.8

I write the following in a file:

foo = c2w

Expected behavior is a code action for importing c2w from Data.ByteString.Internal, instead I receive no hint...

@santiweight santiweight added type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. status: needs triage labels Nov 28, 2022
@michaelpj
Copy link
Collaborator

I think Internal modules are deliberately filtered out.

@konn
Copy link
Collaborator

konn commented Nov 30, 2022

I have also bitten by this feature; this logic also filters out local Internal modules defined in the same package, which hugely sacrifices UX.
I think this feature must be opt-in, not opt-out.

@santiweight
Copy link
Collaborator Author

It makes most sense to have Internal modules as the lowest priority imports, and then only show them when we can't reach the maximum number of imports, imo...

@michaelpj michaelpj added status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet and removed status: needs triage labels Jan 10, 2023
@michaelpj
Copy link
Collaborator

I think @pepeiborra is the proponent of this feature.

@pepeiborra
Copy link
Collaborator

I implemented completions for non-imported modules here: #2040

Internal modules were filtered out because at that time we didn't have a system for sorting completions by importance, and I am not keen on adding user options for something like this.

Contributions are welcome to change and improve the current behaviour.

@michaelpj
Copy link
Collaborator

I'm reading that as you saying that you'd be okay with a PR that re-introduced these completions so long as they were sorted to the bottom?

@mitchellwrosen
Copy link

For what it's I would be in favor of removing all filtering based on sentinel string values even without implementing import ranking. I think it's too fragile of a convention - sometimes Internal appears in a middle-segment rather than at the end, sometimes it's Internals or LowLevel or something else.

@pepeiborra
Copy link
Collaborator

For what it's I would be in favor of removing all filtering based on sentinel string values even without implementing import ranking. I think it's too fragile of a convention - sometimes Internal appears in a middle-segment rather than at the end, sometimes it's Internals or LowLevel or something else.

I would support improving the heuristic, or replacing the filtering with ranking as suggested, but removing it would probably lead to more tickets complaining about internal modules showing up in import lists unexpectedly. CC @fendor

@fendor
Copy link
Collaborator

fendor commented Jan 11, 2023

I agree with pepe, removing the heuristic is probably a bad idea, but ranking them at the very bottom might be an OK middle ground. We did have the completions, and devs started importing the wrong modules more easily, and beginners can't really tell what the contract is of Internal modules and most likely don't know where to even look for these contracts.
Thinking even further, offering these Internal modules at all will motivate beginners to just blindly try to import modules that seem to have the functionality they want. I know, they can do the same via hoogle, but the barrier is noticeably higher, imo.

I agree that filtering on String values is a sub-par solution, but I think currently the benefits for beginners outweighs the drawback for advanced users.
It would be much cooler, if we had some other way to specify the importance of modules.

this logic also filters out local Internal modules defined in the same package, which hugely sacrifices UX.

That is actually something we can fix, right? Iirc, we know whether a module is from this package or a third-party dep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

No branches or pull requests

6 participants