Cleanup addBindingToImportList #942
Conversation
| -- case 2: no children but parentheses, e.g. `import A(Foo(),...)` --> `import A(Foo(Cons), ...)` | ||
| Just ('(', T.uncons -> Just (')', rest'')) -> T.concat [pre, "(", leading, parent, "(", rendered, ")", rest''] | ||
| -- case 3: children with parentheses, e.g. `import A(Foo(ConsA),...)` --> `import A(Foo(Cons, ConsA), ...)` | ||
| Just ('(', T.breakOn ")" -> (children, rest'')) |
There was a problem hiding this comment.
T.stripStart doesn't strip comments, so
import M(Foo({-Cons-}))`
will fall through case 3 and unexpectedly become
import M(Foo(ConsB, {-Cons-})
Not your fault though, your code is as good as it can reasonably be. But the only fully correct way to make edits in the user AST is with ghc-exactprint
There was a problem hiding this comment.
Things become quite complicated involving comments:
- block comments are considered to be constructors
import M(Foo({-Cons-}))⇓
import M(Foo(ConsB, {-Cons-})- unexpected
-}after we matched the parent
import M({-Foo-}Foo)⇓
error "unknown case"- line comments
import M(
--Foo
Foo)⇓
error "unknown case"- ...
It's knotty to resolve these cases simply relying on breakOn logic. As you suggested, manipulating ASTs would be an appropriate way.
|
The tests are unreliable because the code action needs the package exports map to be populated, which is done implicitly by |
|
Thanks! Should we adopt this implementation? If so, how to handle unknown cases? I don't think |
|
I would handle the unknown cases by returning a |
|
@pepeiborra please do. It's only where it is for convenience to me, but always was intended to be upstreamed. |
* Cleanup addBindingToImportList * Remove redundant $ * Fix missing leading identifiers * Simplify * Wait package exports map in tests * Don't show code action if we can't handle this case * Remove redundant parens Co-authored-by: Pepe Iborra <[email protected]>
* Cleanup addBindingToImportList * Remove redundant $ * Fix missing leading identifiers * Simplify * Wait package exports map in tests * Don't show code action if we can't handle this case * Remove redundant parens Co-authored-by: Pepe Iborra <[email protected]>
* Cleanup addBindingToImportList * Remove redundant $ * Fix missing leading identifiers * Simplify * Wait package exports map in tests * Don't show code action if we can't handle this case * Remove redundant parens Co-authored-by: Pepe Iborra <[email protected]>
#916 was tricky and didn't work in some cases. Now the procedure has become clearer, and I add some comments to show how it works. I'm not sure all cases are covered, and we may need more tests.