Closed i-am-tom closed 3 years ago
Nice work Tom!
Thank you for writing this. I like the content, but I just don't know if this is the right place for it. It seems like a good content for a blog post, combined with a shorter doc comment on the ALens
type:
-- | `ALens` is useful for storing a `Lens` in a container or record property,
-- | in order to avoid impredicativity issues in the type checker.
-- |
-- | See [this blog post](...) for more details and a worked example.
or something. Thoughts?
I think it's very much worthwhile including something in the readme about ALens
, as the question about lenses in records happens almost daily. I think this is great, although I'd perhaps want to emphasise that point about records in it, and then expand on it with the deeper explanation perhaps?
Also, assuming we don't want to put the full explanation in the README, adding a docs
to the repo seems more sensible than linking to a blog post I think, since blog links die / fall out of date / are not so easy to find.
Yes, let's make a docs
folder, with a link from the README. I like that idea.
Yep, sounds good! I'll update this PR when I get home :)
This shorter example also fails for me:
type Example = { lens :: Iso' String String }
bad :: Example -> Lens' String String
bad = _.lens
-- Could not match constrained type
-- Profunctor t2 => t2 String String -> t2 String String
-- with type
-- p0 String String -> p0 String String
Looking good, just a couple of suggested edits from me.
Hoping my updates are closer to what you wanted, @paf31!
@garyb has given me permission to merge documentation pull requests (mostly mine), so as to speed up the pace of documentation. (In the case of technical documentation, after someone more experienced gives the thumbs up.)
Unless someone objects, I'll resolve the conflicts and merge this.
I feel like there are still some unaddressed issues here. I'd like to discuss it more.
Also, I think it would be nice to update/replace the example with something like an array of lenses, which I think is easier to understand.
Totally forgot about this! I'll revisit tonight.
If it seems reasonable, I'd like to suggest putting this in the examples
directory that I hope to create (#88).
Suggest renaming the file to something other than Impredicativity
. That's a word from the solution domain, not the problem domain. People will be be visiting this file because they are stumped, so the filename (and, more importantly, the text in the examples README) should be something they'll readily realize is relevant.
@marick I'm quite happy to close this if you want to incorporate it into your work?
It's not an issue I understand. The chances of my understanding it anytime soon are low.
It sounds like this PR is actually quite close to getting merged. There's just a few issues to fix. I found the explanation of impredicativity to be quite clear.
I see a few ways this could move forward, taking in the feedback from others here:
purescript-jordans-reference
, give credit to the original author, and point to this PR for context.docs/
folder.@JordanMartinez I’m in favor of moving this into the docs folder, personally.
@thomashoneyman Sounds fine to me. It's currently in the docs.
As for completing this PR, I think we'll be forced to use the web console. The original user account that created this was deleted, so I don't think this code can be forked and fixed locally.
Ah, I misspoke. It looks like only the fork of this repo was deleted. The user account still exists.
@thomashoneyman How about we merge this as is, and then clean it up in a separate PR?
Nevermind. We can't even merge this. Looks like this will require a new PR completely.
Superceded by #136
Hello, all! We discussed this on Slack - sorry it has taken so long to get round to it... This is my attempt at providing some background on the impredicativity issue. It's a little more "practical" than "complete", but I still think there's some room for improvement, and would love to hear some feedback.
@garyb @natefaubion @paf31 @monoidmusician @LiamGoodacre