-
-
Notifications
You must be signed in to change notification settings - Fork 420
Cursor aware record expansion #4813
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
Conversation
ozkutuk
left a comment
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.
In theory, I like the idea, but unless I am missing something...this doesn't seem to be fixing the original issue at all? I have tried on this example file:
module RecTest where
data RecOuter = RecOuter
{ inner :: RecInner
}
data RecInner
= RecInner
{ foo :: Char
, bar :: Int
}
ex :: RecOuter
ex = RecOuter (RecInner 'c' 42)I have invoked the code action while the cursor was over RecInner, and the result was:
ex :: RecOuter
ex = RecOuter { inner = (RecInner 'c' 42) }...which is already the current behavior and from what I understand exactly what the original issue complains about. I might as well be missing the point of the change though, so please let me know if that is the case. Either way, I think it also makes sense to add a test for the case we are trying to address, both to ensure that the problem is indeed fixed, and also to prevent regressions in the future.
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
c0bcf57 to
d5baf09
Compare
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Show resolved
Hide resolved
fendor
left a comment
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.
Looks pretty good to me, one more test please and a couple of nitpicks!
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
fendor
left a comment
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.
LGTM, thank you!
ozkutuk
left a comment
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.
LGTM, just one minor suggestion on code style. Feel free to take it or leave it as you see fit.
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
aa2c5d5 to
7ec7b65
Compare
|
@vidit-od Looking good! I'll leave it to @fendor to push the merge button.
Yes, I will rebase that PR on top of your work once it gets merged, but thanks for the heads-up. |
Another PR to fix issue #4782. Previously PR #4807 was raised but later discarded due to the following main reason.
This is a Small PR aiming to introduce Cursor aware record expansion.