-
Notifications
You must be signed in to change notification settings - Fork 695
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
Introduce Distribution.Compat.Lens #4701
Conversation
I haven't reviewed the code in detail, but in general I'm +1 on this idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Maybe we can come up with some scripting to reduce the manual lens-derivation maintenance effort for the large product types, but for now this is good enough for me.
Cabal/Distribution/Compat/Lens.hs
Outdated
-- Common | ||
------------------------------------------------------------------------------- | ||
|
||
_2 :: Functor f => (a -> f b) -> (c, a) -> f (c, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't say _2
w/o saying _1
, IMO ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is btw a good question. Should we introduce FieldN
classes for these, like lens
and microlens
do? (I have _4
used somewhere already)
-- | ||
-- First start a repl with proper-lens dependency | ||
-- | ||
-- > cabal new-repl Cabal:lib:Cabal ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the documented but not yet implemented extra-packages:
is supposed to provide just that; but until it is implemented, a cabal flag
if flag(deploy-lens-missiles)
build-depends: lens >= ... && < ...
is a reasonable temporary workaround.
PS: I already have an idea, how we can generate the code w/o dragging in lens
nor requiring TH during the generation time. Just needs an afternoon to implement...
This commit includes some lens defintions to show up, how the Cabal-lens framework could look like. As an example checks which used ad-hoc lenses use newly introduced lens-framework.
@23Skidoo seems I cannot restart appveyor builds, can you help? |
Restarted the build. IIRC @hvr said that he could work around that by logging in to AppVeyor with my user name somehow. |
Yeah, for some reason I get offered a dropdown allowing me impersonate @23Skidoo or just be myself... |
It's just AppVeyor being weird and having no good equivalent for Github organizations. I think that if @phadej was added to the |
AppVeyor failure seems legit, BTW. |
and now it isn't legit:
|
Restarted. |
This patch introduces
Distribution.Compat.Lens
, a small portion oflens
. The need is two fold:Cabal
Check.hs
)The design is simple:
Distribution.Compat.Lens
have some definitions fromlens
..Lens
module which exports the type and lenses, with the same name as fields. Users can eitheror
Latter variant is not yet viable, as not everything is lensified. But we can make progress there quite easily.
To better compare lens and "old-school" Haskell approaches see e.g. (I realized later, these are essentially same checks):
checkUnusedFlags
https://github.com/phadej/cabal/blob/2dbd57a4aba07828c9520c7692c63138cfcda658/Cabal/Distribution/PackageDescription/Check.hs#L1620-L1642checkConditionals
https://github.com/phadej/cabal/blob/2dbd57a4aba07828c9520c7692c63138cfcda658/Cabal/Distribution/PackageDescription/Check.hs#L1561-L1597This commit includes some lens defintions to show up, how the Cabal-lens
framework could look like.
As an example checks which used ad-hoc lenses use newly introduced
lens-framework.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!