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.

Sign up for GitHub

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

Jump to bottom

Remove undefined exports suggestions #1193

Merged
merged 2 commits into from
Jan 23, 2021
Merged

Remove undefined exports suggestions #1193

merged 2 commits into from
Jan 23, 2021

Conversation

kderme
Copy link
Contributor

@kderme kderme commented Jan 11, 2021 •
edited
Loading

This pr adds suggestions for removing elements of the export list causing an error or a warning. It also adds a special suggestion to remove all such elements at once:

hls2

It looked a lot like the remove imports suggestions, so I tried to use things from there, but this wasn't always possible.

@kderme kderme changed the title Remove undefined exports Remove undefined exports suggestions Jan 11, 2021
pepeiborra
pepeiborra approved these changes Jan 11, 2021
View reviewed changes
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very nice work, solidly tested. Thanks!

Ailrun
Ailrun reviewed Jan 11, 2021
View reviewed changes
ghcide/src/Development/IDE/Plugin/CodeAction.hs Outdated
@@ -100,7 +101,8 @@ codeAction lsp state (TextDocumentIdentifier uri) _range CodeActionContext{_diag
[ CACodeAction $ CodeAction title (Just CodeActionQuickFix) (Just $ List [x]) (Just edit) Nothing
| x <- xs, (title, tedit) <- suggestAction exportsMap ideOptions parsedModule text x
, let edit = WorkspaceEdit (Just $ Map.singleton uri $ List tedit) Nothing
] <> caRemoveRedundantImports parsedModule text diag xs uri
] <> caRemoveRedundantExports parsedModule text diag xs uri
Copy link
Member

@Ailrun Ailrun Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind if I request to use a different name?
I mean, those undefined exports are not something "redundant", and it's more like, just as your title, "undefined", right?
I found this name is a bit confusing and may conflict with the real "removing redundant export" feature in the future.

Copy link
Contributor Author

@kderme kderme Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed the name should change. It covers a bit more cases than undefined, like dodgy-exports. Maybe wrongExports or invalidExports?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they would be better.

@jneira
Copy link
Member

jneira commented Jan 12, 2021

I think this pr will close #156

@jneira jneira linked an issue Jan 12, 2021 that may be closed by this pull request
Add code action for hiding imports #156
Closed
@jneira
Copy link
Member

jneira commented Jan 14, 2021

hlint is failing:

Downloading and running hlint...
#=#=#                                                                         

######################################################################## 100.0%
                                                                           0.0%
######################################################################## 100.0%
ghcide/src/Development/IDE/Plugin/CodeAction.hs:237:18-20: Warning: Use nubOrd
Found:
  nub
Perhaps:
  nubOrd

ghcide/src/Development/IDE/Plugin/CodeAction.hs:254:53-55: Warning: Use nubOrd
Found:
  nub
Perhaps:
  nubOrd

ghcide/src/Development/IDE/Plugin/CodeAction.hs:264:53-55: Warning: Use nubOrd
Found:
  nub
Perhaps:
  nubOrd

ghcide/src/Development/IDE/Plugin/CodeAction/PositionIndexed.hs:120:5-41: Suggestion: Use list comprehension
Found:
  if acceptNoComma then [range] else []
Perhaps:
  ([range | acceptNoComma])

4 hints
Error: Process completed with exit code 1.

@jneira
Copy link
Member

jneira commented Jan 18, 2021

It needs a rebase too

@kderme kderme force-pushed the remove-export branch 2 times, most recently from 906cae7 to b141ffb Compare January 20, 2021 22:07
Ailrun
Ailrun approved these changes Jan 22, 2021
View reviewed changes
Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to go? @pepeiborra

pepeiborra
pepeiborra approved these changes Jan 23, 2021
View reviewed changes
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Jan 23, 2021
@mergify mergify bot merged commit 759901f into haskell:master Jan 23, 2021
@kderme kderme deleted the remove-export branch January 23, 2021 10:45
@pepeiborra pepeiborra mentioned this pull request Jan 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ailrun Ailrun Ailrun approved these changes

@pepeiborra pepeiborra pepeiborra approved these changes

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add code action for hiding imports
4 participants
@kderme @jneira @pepeiborra @Ailrun