Skip to content

Redundant imports/exports: use range only to determine which code actions are in scope#4063

Merged
fendor merged 7 commits into
haskell:masterfrom
keithfancher:fix-remove-redundant-imports-range-only
Feb 21, 2024
Merged

Redundant imports/exports: use range only to determine which code actions are in scope#4063
fendor merged 7 commits into
haskell:masterfrom
keithfancher:fix-remove-redundant-imports-range-only

Conversation

@keithfancher

@keithfancher keithfancher commented Feb 9, 2024

Copy link
Copy Markdown
Contributor

...rather than doing a full compare with incoming Diagnostic objects from the client.

This brings the "remove redundant imports/exports" code actions more in line with behavior described in #4056, and has the pleasant side-effect of fixing broken code actions in neovim (#3857).

Manually tested removing redundant imports and exports in both neovim and VS Code -- all working as expected!

Rather than doing a full compare with incoming `Diagnostic` objects from
the client. This brings the "remove redundant imports/exports" code
actions more in line with behavior described in haskell#4056, and has the
pleasant side-effect of fixing broken code actions in neovim (haskell#3857).
@keithfancher

Copy link
Copy Markdown
Contributor Author

What do y'all think? Is this good to go, or any other open questions? Would probably be good for @michaelpj to take a look as well, to make sure I'm getting the intent of #4056 correct.

I'd love to get #3857 closed out, it's such an annoying bug for neovim users...

@fendor fendor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@keithfancher

Copy link
Copy Markdown
Contributor Author

LGTM

Awesome, thank you!

As for getting it merged... do I just hang tight and wait for a maintainer to pull the trigger?

@fendor fendor added the status: needs review This PR is ready for review label Feb 21, 2024
@fendor
fendor enabled auto-merge (squash) February 21, 2024 21:50
@michaelpj

Copy link
Copy Markdown
Collaborator

Sorry, I am going to take a look at this! I was on holiday last week and it hasn't quite got to the top of my pile yet :)

@keithfancher

Copy link
Copy Markdown
Contributor Author

Sorry, I am going to take a look at this! I was on holiday last week and it hasn't quite got to the top of my pile yet :)

Oh, no worries at all, take your time of course! I just wanted to be sure I wasn't missing some step on my end.

Thank you!

@fendor
fendor merged commit af393d6 into haskell:master Feb 21, 2024
soulomoon pushed a commit to soulomoon/haskell-language-server that referenced this pull request Feb 23, 2024
…ions are in scope (haskell#4063)

* Use *only* incoming range to determine which code actions are in scope

Rather than doing a full compare with incoming `Diagnostic` objects from
the client. This brings the "remove redundant imports/exports" code
actions more in line with behavior described in haskell#4056, and has the
pleasant side-effect of fixing broken code actions in neovim (haskell#3857).

* Remove redundant imports ;)

* Rename param for clarity

---------

Co-authored-by: fendor <fendor@users.noreply.github.com>

@michaelpj michaelpj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great to me btw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review This PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants