-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
feat(compiler): suggest better suggestions for imports on unknown var… #2715
feat(compiler): suggest better suggestions for imports on unknown var… #2715
Conversation
a2f9ff8
to
5aa4483
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.
Nice! Thank you! The code looks nice and clean.
We'll need some tests though! To ensure the functionality works and that there are no regressions in future.
I have a feeling that while this works well for io.debug
it'll not work so well for modules with longer names. The module.
is going to have a big impact on the edit distance so they won't ever be selected as a suggestion. We can write some tests to verify this, but if it turns out to be the case we'll need to use a different approach, like the one I suggested here #2697 (comment).
Thanks again! Very nice work
.map(|value| format!("{}.{}", module_name, value).into()) | ||
.collect::<Vec<EcoString>>() |
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.
.map(|value| format!("{}.{}", module_name, value).into()) | |
.collect::<Vec<EcoString>>() | |
.map(move |value| format!("{}.{}", module_name, value).into()) |
Regarding the ci-lint error about flat_map
, you can additionally avoid the intermediate .collect()
s by moving the module_name
reference into the closure. Its lifetime should last fine for the duration of this whole function (or otherwise the borrow checked would yell).
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.
Could use that cool eco_format macro 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.
Thanks for the feedback, appreciated. I will update this 🙂
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 think you missed the main part of my suggestion: removing the .collect::<Vec<EcoString>>()
. Adding the move
just enables that :)
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.
Updated it, Looks good to you? Thanks :)
Yes definitely, it works well for this small example but this implementation might quickly shows his limits with more complex examples. I will continue to look on how to improve this and find a better approach that handle more complex suggestions. If anyone else have some ideas or code solutions, I would be glad to take this as part of this PR. Thanks 🙂 |
…iable This improves the overall compiler unknown variables suggestions by listing all imported modules values in the scope so it can now recommends imports. For example with the following code (described in GitHub issue gleam-lang#2697): ``` import gleam/io pub fn main() { let numbers = [1, 2, 3, 4, 5] debug(numbers) // Intended to use io.debug but mistakenly omitted 'io.' } ``` The current version of the compiler provides the following error: ``` error: Unknown variable ┌─ /path/to/project/src/my_project.gleam:5:3 │ 5 │ debug(numbers) │ ^^^^^ Did you mean `True`? ``` With my changes the suggestions are a bit smarter: ``` error: Unknown variable ┌─ /Users/shellbear/code/gleamio/src/gleamio.gleam:5:3 │ 5 │ debug(numbers) // Intended to use io.debug but mistakenly omitted 'io.' │ ^^^^^ Did you mean `io.debug`? The name `debug` is not in scope here. ```
28a80b8
to
77a74df
Compare
Closing due to inactivity. Please feel free to reopen! Thank you |
This improves the overall compiler unknown variables suggestions by listing all imported modules values in the scope so it can now recommends imports.
For example with the following code (described in the following GitHub issue):
The current version of the compiler provides the following error:
With my changes the suggestions are a bit smarter:
It's my first contribution to the project so I would be really happy to receive any feedback or improvement. I didn't have a look at the full codebase, it was more a first attempt to implement this improvement and learn more about Gleam compiler.
Maybe some unit tests for these changes would be great.
Thanks ✌️
closes #2697