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.

Sign up for GitHub

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

Jump to bottom

Restore BC for rewriteFile #4996

Merged
merged 2 commits into from Jan 4, 2018
Merged

Restore BC for rewriteFile #4996

merged 2 commits into from Jan 4, 2018

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Jan 4, 2018

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!

@ezyang
s/rewriteFile/rewriteFileV/
e824882 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Restore BC compatibility for rewriteFile, fixes haskell#4981
a3e1833 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang ezyang requested a review from erikd January 4, 2018 03:51
erikd
erikd approved these changes Jan 4, 2018
View reviewed changes
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Thanks @ezyang . LGTM.

@23Skidoo 23Skidoo merged commit 587bc30 into haskell:master Jan 4, 2018
@23Skidoo
Copy link
Member

23Skidoo commented Jan 4, 2018

Merged, thanks!

@hvr
Copy link
Member

hvr commented Jan 4, 2018 •
edited

@23Skidoo I'm utterly confused now... are you aware of @phadej's 5797013 & #4475?

@23Skidoo
Copy link
Member

23Skidoo commented Jan 4, 2018 •
edited

Did I forget to cherry-pick that into master? Then I guess we should rename rewriteFileV to rewriteFileEx.

@hvr
Copy link
Member

hvr commented Jan 4, 2018 •
edited

@23Skidoo what's the difference between rewriteFileV and Cabal 2.0's rewriteFileEx; and why would we need a 3rd rewriteFile'? Why don't we just revert this one, and instead cherry-pick 5797013 ?

23Skidoo added a commit that referenced this pull request Jan 4, 2018
@23Skidoo
Rename rewriteFileV to rewriteFileEx.
552ae22 
See
#4996 (comment). Apparently
I forgot to cherry-pick 5797013 from
2.0 to master.
@phadej
Copy link
Collaborator

phadej commented Jan 4, 2018

Aren't rewriteFileV and rewriteFileEx the same function. I don't care about the name, but let's use rewriteFileEx as it's in released Cabal already.

My bad, I didn't backport myself either.

@23Skidoo
Copy link
Member

23Skidoo commented Jan 4, 2018

Fixed in 552ae22.

@phadej
Copy link
Collaborator

phadej commented Jan 4, 2018

@23Skidoo thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikd erikd erikd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ezyang @23Skidoo @hvr @phadej @erikd