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

Re-export #1058 operators from Control.Lens.Operators, fix incorrect documentation #1068

Merged
merged 5 commits into from
May 12, 2024

Conversation

RyanGlScott
Copy link
Collaborator

@RyanGlScott RyanGlScott commented May 11, 2024

Fixes #1067.

Copy link
Contributor

@endgame endgame left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this up, I can only see one doc nit. While we're here, I think we should add the << variants (which return the old result), because the consistency of lens' operator language is one of its greatest strengths. They're also fairly simple to implement.

src/Control/Lens/Lens.hs Outdated Show resolved Hide resolved
This prepend to the front rather than appending at the back. Spotted in #1067.
@RyanGlScott
Copy link
Collaborator Author

While we're here, I think we should add the << variants (which return the old result)

What names would you propose for these? We're in a bit of a weird situation as some of these operators already begin with < and <<, so the usual convention of adding << to the front of the name to indicate returning the old result doesn't work for all of these operators.

@endgame
Copy link
Contributor

endgame commented May 12, 2024

I don't think that's a blocker: we already have (<<<>=) for "<> onto part of a MonadState and return the old value".

There are only really three new operators in 5.3.1:

  • (<>:~) (prepend front via (<>))
  • (<|~) (cons via (<|))
  • (|>~) (snoc via (|>))

The PR that added them added the the "return new result variants" (beginning with <) and the MonadState variants (s/~/=). I am just suggesting we fill out the rest of the established pattern:

  • (<<<>:~) (prepend front via (<>) and return old result)
  • (<<<>:=) (prepend front via (<>) and return old result, MonadState version)
  • (<<<|~) (prepend front via (<|) and return old result)
  • (<<<|=) (prepend front via (<|) and return old result, MonadState version)
  • (<<|>~) (append back via (|>) and return old result)
  • (<<|>=) (append back via (|>) and return old result, MonadState version)

Personally, I have never really needed the operators in Control.Lens.Cons and would have expected <|>~ to mean "combine the target of the lens with a new value according to its Alternative instance", but since it's in lens now it's probably best to build out the matching operator variants for consistency.

@RyanGlScott
Copy link
Collaborator Author

I've pushed a commit which implements the operators suggested in #1068 (comment). Let me know if this covers everything.

Copy link
Contributor

@endgame endgame left a comment

Choose a reason for hiding this comment

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

Seems right to me. Just some whitespace lint which you can take or leave.

Thanks for tidying this up!

src/Control/Lens/Lens.hs Outdated Show resolved Hide resolved
src/Control/Lens/Lens.hs Outdated Show resolved Hide resolved
src/Control/Lens/Lens.hs Outdated Show resolved Hide resolved
src/Control/Lens/Lens.hs Outdated Show resolved Hide resolved
@RyanGlScott RyanGlScott merged commit 472dfa1 into master May 12, 2024
26 checks passed
RyanGlScott added a commit that referenced this pull request May 12, 2024
@RyanGlScott RyanGlScott deleted the T1067 branch May 12, 2024 22:06
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.

New operators not exported by Control.Lens.Operators
2 participants