-
Notifications
You must be signed in to change notification settings - Fork 57
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
add broken test for pipe completion on aliased types #700
Conversation
It needs to start from the type environment and expand the definition. |
…when resolving aliased types
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 one!
| Tlink t1 | Tsubst t1 | Tpoly (t1, []) -> | ||
digToRelevantType ~env ~package t1 | ||
(* Don't descend into types named "t". Type t is a convention in the ReScript ecosystem. *) | ||
| Tconstr (path, _, _) when path |> Path.last = "t" -> (env, TypExpr t) |
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.
What happens in practice if this case is omitted?
I was thinking of the different cases. E.g. when it's an alias to array
, whether one wants to complete for array
. And when it's a record definition, then one wants to treat is as the source of truth.
Just thinking aloud here, about what the convention could look like.
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.
Yeah, I added this case because it broke 2 tests in Completion.res
, and looking at them I felt like the old behavior was better.
Yeah, I agree. I don't feel like there's a clear and good answer here, but I do lean a bit towards the current behavior, as in type t
usually means the main type, and if it's in a module there's often functions in the module to work on it. But I agree it's not 100%.
One idea could be to complete for the module with the type t
, and the aliased value. Unsure if that'd be messy to implement, but it might be good behavior. What do you think?
Merging since it doesn't change current behavior.
Pipe completion does not work on aliased types. Wondering what the best way would be to make it work.