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.

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

Wingman: Fix TODO(sandy) when performing subsequent actions #2580

Merged
merged 5 commits into from Jan 13, 2022

Conversation

isovector
Copy link
Collaborator

@isovector isovector commented Jan 12, 2022

This PR fixes #2448, by using Uri internally instead of NormalizedFilePath. It's not clear why it fixes it, since all the functions involved are pure, but who knows. Huge shoutouts to @pepeiborra for tracking down the issue in the first place.

, fc_range :: Maybe (Tracked 'Current Range)
-- ^ For code actions, this is 'Just'. For code lenses, you'll get
-- a 'Nothing' in the request, and a 'Just' in the response.
}
deriving stock (Eq, Ord, Show, Generic)
deriving anyclass (A.ToJSON, A.FromJSON)

deriving anyclass instance A.ToJSON NormalizedFilePath
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is pretty obvious when you look at these :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe there should be an instance TypeError "Do not do this" => ToJSON NormalizedFilePath definition in the lsp package where NormalizedFilePath is defined, to prevent this weird bug from happening again

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to add that - what goes wrong with it OOI?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't remember anymore. Presumably the derived instances are not roundtripping, i.e. fromJSON . receiveLSP . sendLSP . toJSON /= id, because of the embedded hashes.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks both for the fix

@isovector isovector changed the title Wingman: Fix TODO(sand) when performing subsequent actions Wingman: Fix TODO(sandy) when performing subsequent actions Jan 12, 2022
@isovector isovector added the merge me Label to trigger pull request merge label Jan 13, 2022
@mergify mergify bot merged commit 18a8996 into master Jan 13, 2022
@isovector isovector deleted the fix-nfp-wingman branch January 17, 2022 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wingman: TODO(sandy) error pops up after the first tactic
4 participants