search

well-typed / optics

Optics as an abstract interface
375 stars 24 forks source link

Add is #410

Closed tomjaguarpaw closed 3 years ago

tomjaguarpaw commented 3 years ago

(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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/well-typed/optics/pull/411.~

phadej commented 3 years ago

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 commented 3 years ago

@adamgundry, @arybczak what you think about this?

tomjaguarpaw commented 3 years ago

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

arybczak commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

(I'm not sure why the doctest is failing. I can't reproduce locally due to https://github.com/well-typed/optics/issues/413).

arybczak commented 3 years ago

(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.

arybczak commented 3 years ago

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

adamgundry commented 3 years ago

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 commented 3 years ago

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

arybczak commented 3 years ago

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