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

Switch to normal field selectors and generic-lens #562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelpj
Copy link
Collaborator

This adopts the approach discussed here:
#465 (comment)

That is:

  • We export normal, non-prefixed record selectors (still using DuplicateRecordFields, of course).
  • Users who want lenses can use generic-lens; lsp and lsp-test do this.
  • It's sensible for lsp-types to define some useful lenses that aren't derived from fields; these go in a lsp-types-lens component.

I think the result is... fine?
kcsongor/generic-lens#96 is a pain in some cases, but by and large using the generic lenses is quite nice.

I also tried to just use OverloadedRecordDot instead of lenses where I could, since we now support 9.2 as our earliest version. I couldn't quite get rid of lens in lsp, it's too useful. I did get rid of it entirely in lsp-types, which was quite painful in at least one place.

This would obviously be a huge breaking change, but I think it's the right direction.

This adopts the approach discussed here:
#465 (comment)

That is:
- We export normal, non-prefixed record selectors (still using
  `DuplicateRecordFields`, of course).
- Users who want lenses can use `generic-lens`; `lsp` and `lsp-test` do
  this.
- It's sensible for `lsp-types` to define some useful lenses that aren't
  derived from fields; these go in a `lsp-types-lens` component.

I think the result is... fine?
kcsongor/generic-lens#96 is a pain in some
cases, but by and large using the generic lenses is quite nice.

I also tried to just use `OverloadedRecordDot` instead of lenses where I
could, since we now support 9.2 as our earliest version. I couldn't
quite get rid of `lens` in `lsp`, it's too useful. I did get rid of it
entirely in `lsp-types`, which was quite painful in at least one place.

This would obviously be a huge breaking change, but I think it's the
right direction.
@s-and-witch
Copy link

I didn't go too much into the code, but after looking at several files, I can see that we use a very limited set of lens API, like has, ^. and ^?. It's possible to declare these functions just in-place, so we would be able to get rid of lens dependency entirely. So, the user would be free to choose between lens, microlens or just don't pay for what he will not use in his code.

Entire code required for declaring these functions:

type Getting r s a = (a -> Const r a) -> s -> Const r s

foldMapOf :: Getting r s a -> (a -> r) -> s -> r
foldMapOf = coerce

has :: Getting Any s a -> s -> Bool
has l = getAny . foldMapOf l (\_ -> Any True)

(^.) :: s -> Getting a s a -> a
s ^. l = getConst (l Const s)

(^?) :: s -> Getting (First a) s a -> Maybe a
s ^? l = getFirst (foldMapOf l (First . Just) s)

@michaelpj
Copy link
Collaborator Author

Right, so this PR drops the lens dependency from lsp-types. What you suggest would let us drop it maybe also from lsp and lsp-test as well. Although I think it might be cleaner to depend on microlens, since it's whole purpose is to be more minimal.

We still need the generic-lens dependency to create the lenses, but generic-lens doesn't actually depend on lens, so that wouldn't be too bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants