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

Add is #410

Merged
merged 2 commits into from
Mar 28, 2021
Merged

Add is #410

merged 2 commits into from
Mar 28, 2021

Conversation

tomjaguarpaw
Copy link
Contributor

(the opposite of isn't)

I found it rather strange that isn't exists but is doesn't. lens has is. Could we add it to optics?

@phadej
Copy link
Contributor

phadej commented Mar 6, 2021

lens has is in the Control.Lens.Extras module, which isn't re-exported through Control.Lens.

Also optics has some is in type-signatures. That would be confusing. ;)

I'd rather do nothing, i.e. don't add this combinator.

@tomjaguarpaw
Copy link
Contributor Author

tomjaguarpaw commented Mar 6, 2021

Also optics has some is in type-signatures. That would be confusing. ;)

I provided the link to lens for informational purposes, not for justification! The justification was that it was sufficiently confusing to me that is does not exist. I imagine that less experienced users would be even more confused.

I don't buy the argument that it will be confused with Is. It is well-enough established that capitalized identifiers are distinct from uncapitalized ones. However, if you really do hold strongly to that claim can you please memorialise your rationale in the Haddocks as per #411.

@phadej
Copy link
Contributor

phadej commented Mar 6, 2021

I didn't mean that we shouldn't have is. I was against exporting it from Optics.AffinedFold and thus transitively Optics.Core and Optics.

And the confusion I was referring to was not with Is type family, but with is as plural i. It is a name-clash risky name. Therefore I'd prefer not to have it exported from Optics module.

@phadej
Copy link
Contributor

phadej commented Mar 6, 2021

@adamgundry, @arybczak what you think about this?

@tomjaguarpaw
Copy link
Contributor Author

tomjaguarpaw commented Mar 6, 2021

OK, thanks for clarifying. Avoiding export from top level make perfect sense to me. What would be the right way to acheve that?

@arybczak
Copy link
Collaborator

arybczak commented Mar 6, 2021

I'd like is to be added to some sort of Extras module similar to how lens does it.

I recently wanted to use it (with a prism), but had to settle for has :(

@adamgundry
Copy link
Member

I agree that we shouldn't export is from Optics or Optics.Core. I'm not opposed to exporting it from another module if we can think of a name. We have Optics.Extra already so we need something different.

Perhaps we should have a module for "things that aren't exported because of name collisions" and then it could also contain (.) = (%)...

Although given that has/hasn't are more general, do we actually gain much from having is/isn't as well? We could just document the availability of the former?

@arybczak
Copy link
Collaborator

arybczak commented Mar 7, 2021

We have Optics.Extra already so we need something different.

Optics.Core.Extras sounds good to me.

Although given that has/hasn't are more general, do we actually gain much from having is/isn't as well?

Testing for a particular type constructor reads nicer as is #Con than has #Con ;) Though that's the only situation I encountered where I'd like is.

@tomjaguarpaw
Copy link
Contributor Author

OK, how about this? I chose Optics.Core.Extra (without an s) instead of Optics.Core.Extras to match Optics.Extra. Let me know if you prefer me to switch that.

Regarding is/isn't, has/hasn't, I definitely prefer to have the former because they read better and they guarantee to me that the argument Is an AffineFold not just a Fold, which I think increases legibility. On the other hand if isn't is provided then I think it's really important to also provide is, to minimise bamboozlement.

@tomjaguarpaw
Copy link
Contributor Author

(I'm not sure why the doctest is failing. I can't reproduce locally due to #413).

@arybczak
Copy link
Collaborator

(I'm not sure why the doctest is failing. I can't reproduce locally due to #413).

Because the file is missing

-- $setup
-- >>> import Optics.Core

at the end.

@@ -147,6 +147,8 @@ infixl 3 `afailing` -- Same as (<|>)
-- >>> isn't _Just Nothing
-- True
--
-- The negation of this operator is 'Optics.Core.Extra.is' from
-- "Optics.Core.Extra".
isn't :: Is k An_AffineFold => Optic' k is s a -> s -> Bool
isn't k s = not (isJust (preview k s))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be isNothing. Can you please change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

)
where

import Optics.Internal.Optic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only Optics.Optic is needed, no Internal stuff is used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

-- False
--
is :: Is k An_AffineFold => Optic' k is s a -> s -> Bool
is k = not . isn't k
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you inline the definition of isn't here so that the compiler has less things to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@arybczak
Copy link
Collaborator

Thanks, looks good to merge to me 👍 Unless @adamgundry @phadej would like the module to have a different name.

@adamgundry
Copy link
Member

Thanks for taking care of this @tomjaguarpaw!

I'm somewhat torn on the module name question, because Optics.Extra exists already as an index of things in the optics-extra package, which is rather unrelated to Optics.Core.Extra. The latter is most closely related to Control.Lens.Extras, of course. I don't have a compelling answer, but I think I'd prefer either Optics.Core.Extras (to match lens) or perhaps a completely different synonym such as Optics.Core.Surplus.

@tomjaguarpaw
Copy link
Contributor Author

OK, I've changed the module name to Optics.Core.Extras.

@arybczak
Copy link
Collaborator

Thanks :) I'll merge after the weekend unless anyone objects.

@arybczak arybczak merged commit 6d4b0a8 into well-typed:master Mar 28, 2021
@tomjaguarpaw tomjaguarpaw deleted the is branch March 29, 2021 09:10
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.

4 participants