Skip to content

RFC: Potential fix for #4808 #4830

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Oct 26, 2017
Merged

RFC: Potential fix for #4808 #4830

merged 1 commit into from
Oct 26, 2017

Conversation

hvr
Copy link
Member

@hvr hvr commented Oct 18, 2017

See #4808 (comment)

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

@hvr hvr requested review from phadej and 23Skidoo October 18, 2017 21:01
@phadej
Copy link
Collaborator

phadej commented Oct 18, 2017

Test would be great.

I can only shake my head around this. If only we could have versioned APIs to call older Setup.hs.

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

Looks OK, but I'd like to see a test. We can merge it as-is to 2.0 to get the point release out faster, and leave the PR for master open.

@hvr
Copy link
Member Author

hvr commented Oct 21, 2017

@23Skidoo I'm not sure yet how such a test would look like; we can probably do easy roundtrip tests (and it would help if FlagAssignment was a newtype, so we could attach parse/display instances to it).

(relatedly, @ezyang mentioned in #4710 (comment) we don't have many communication tests with different lib:Cabal versions)

So which kind of tests did you (& @phadej) have in mind specifically?

@bgamari
Copy link
Contributor

bgamari commented Oct 25, 2017

What is the status of this? For the record, this is something that 8.2.2 is blocking on.

@phadej
Copy link
Collaborator

phadej commented Oct 26, 2017

@hvr, I'm ok for this going to 2.0 as is, but for master could we change FlagAssignment to the newtype and do roundtrip stuff?

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

See my comment.

@phadej phadej merged commit bb5e43f into haskell:master Oct 26, 2017
@phadej
Copy link
Collaborator

phadej commented Oct 26, 2017

cherry-picked on 2.0. @bgamari could you verify it unblocks GHC-8.2.2?

@hvr
Copy link
Member Author

hvr commented Oct 26, 2017

but for master could we change FlagAssignment to the newtype and do roundtrip stuff?

oh yes; in fact I have already started on refactoring it into a newtype (see #4849) to see how much places I'd have to touch; it's not bad at all :-)

@hvr hvr deleted the pr/issue4808 branch October 26, 2017 10:06
@bgamari
Copy link
Contributor

bgamari commented Oct 26, 2017

Thanks @phadej. That should do it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants