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

Turn 'configPrograms' field into a 'Last'-monoid #3195

Merged
merged 1 commit into from
Feb 27, 2016

Conversation

hvr
Copy link
Member

@hvr hvr commented Feb 27, 2016

This implements the suggestions mentioned at
#3169 (comment)

The main benefit of this change is turning 'ConfigFlags' into a uniform
product-type suitable for generic derivation of pointwise
Semigroup/Monoid instances.

NB: This changes the Binary serialisation of ConfigFlags since there's
now an additional Maybe inserted in configPrograms's type

@hvr hvr force-pushed the pr/configprograms-last branch 2 times, most recently from 040ac42 to c9e08f9 Compare February 27, 2016 16:47
@23Skidoo
Copy link
Member

Still fails (on < 7.6):

Distribution/Simple/Setup.hs:397:10:
    No instance for (Binary (Last' ProgramConfiguration))
      arising from a use of `Distribution.Compat.Binary.Class.$gdmput'
    Possible fix:
      add an instance declaration for
      (Binary (Last' ProgramConfiguration))
    In the expression: (Distribution.Compat.Binary.Class.$gdmput)
    In an equation for `put':
        put = (Distribution.Compat.Binary.Class.$gdmput)
    In the instance declaration for `Binary ConfigFlags'

@hvr
Copy link
Member Author

hvr commented Feb 27, 2016

@23Skidoo that one's weird... I'm investigating...

This implements the suggestions mentioned at
haskell#3169 (comment)

The main benefit of this change is turning 'ConfigFlags' into a uniform
product-type suitable for generic derivation of pointwise
`Semigroup`/`Monoid` instances.

NB: This changes the `Binary` serialisation of `ConfigFlags` since there's
now an additional `Maybe` inserted in `configPrograms`'s type
@hvr hvr force-pushed the pr/configprograms-last branch from c9e08f9 to 62c3aa6 Compare February 27, 2016 20:07
@hvr
Copy link
Member Author

hvr commented Feb 27, 2016

@23Skidoo I'm quite optimistic the builds will work this time...

@@ -404,7 +404,7 @@ configAbsolutePaths f =

defaultConfigFlags :: ProgramConfiguration -> ConfigFlags
defaultConfigFlags progConf = emptyConfigFlags {
configPrograms = progConf,
configPrograms = pure progConf,
Copy link
Member

Choose a reason for hiding this comment

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

Last' progConf would be more readable IMO.

23Skidoo added a commit that referenced this pull request Feb 27, 2016
Turn 'configPrograms' field into a 'Last'-monoid
@23Skidoo 23Skidoo merged commit 5a77348 into haskell:master Feb 27, 2016
@23Skidoo
Copy link
Member

Merged, thanks! I'll need to check whether this patch breaks any setup scripts before it can be cherry-picked into 1.24 as well (ConfigFlags are used by hooks, but I don't think hooks can make use of configPrograms).

@hvr
Copy link
Member Author

hvr commented Feb 27, 2016

@23Skidoo IIRC @dcoutts was convinced that changing the internal representation of ConfigFlags shouldn't break user-hooks

@hvr hvr deleted the pr/configprograms-last branch February 27, 2016 21:10
@23Skidoo
Copy link
Member

Yes, I don't think it does, but it never hurts to double-check.

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.

2 participants