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

Piyush whitespace issue #5514

Merged
merged 2 commits into from
Mar 2, 2019
Merged

Piyush whitespace issue #5514

merged 2 commits into from
Mar 2, 2019

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Aug 11, 2018 •
edited
Loading

Rebased to current (2019-03-02) master

Unconflicts and improves over #5293

@phadej phadej requested a review from hvr August 11, 2018 11:07
quasicomputational
quasicomputational reviewed Aug 11, 2018
View reviewed changes
Copy link
Contributor

@quasicomputational quasicomputational left a comment

Choose a reason for hiding this comment

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

Travis doesn't like it and that looks legit.

Needs a mention in the file format changelog and to have the specific mention of the whitespace parsing bug in the docs (in developing-packages.rst, I think, in the mixins field description) removed.

Cabal/tests/ParserTests/regressions/mixin-2.cabal Outdated Show resolved Hide resolved
Cabal/tests/ParserTests/regressions/mixin-2.cabal Show resolved Hide resolved
@phadej
Copy link
Collaborator Author

phadej commented Aug 11, 2018 •
edited
Loading

I just resolved the conflict of previous PR, and added tests which should been there to begin with. If someone really want this for 2.4, they can resolve the remaining issues.

@hvr hvr added this to the 2.4 milestone Aug 11, 2018
@piyush-kurur
Copy link
Contributor

@phadej what are the missing things now? I can try to see if I can contribute so that we have this in 2.4

@quasicomputational
Copy link
Contributor

@piyush-kurur There was a failure on Travis but I don't see it now, so maybe that's been fixed. There are only two things that I think need doing now:

  • analogous tests for the hiding form, which have also changed (i.e., make sure whitespace breaks the 2.2 parser but not the 2.4 one)
  • and docs + changelogs

@phadej phadej modified the milestones: 2.4, 3.0 Aug 20, 2018
@phadej
Copy link
Collaborator Author

phadej commented Dec 4, 2018

Ah, I should fix this for 3.0

@phadej phadej changed the base branch from 2.4 to master March 2, 2019 16:01
@phadej
Copy link
Collaborator Author

phadej commented Mar 2, 2019

Rebased on current master, let's get this in for 3.0

@phadej phadej merged commit eb38139 into haskell:master Mar 2, 2019
@phadej phadej deleted the piyush-whitespace-issue branch March 2, 2019 18:54
This was referenced Mar 2, 2019
Closed
Closed
@piyush-kurur
Copy link
Contributor

Aha finally my issues with white space has been cured ;-)

@23Skidoo 23Skidoo mentioned this pull request Mar 3, 2019
Closed
This was linked to issues Feb 15, 2021
The Parser for ModuleRenaming fails to skip some spaces #4864
Closed
Parse error on "mixins" field #5150
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quasicomputational quasicomputational quasicomputational left review comments

@hvr hvr hvr left review comments

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

Successfully merging this pull request may close these issues.

Parse error on "mixins" field The Parser for ModuleRenaming fails to skip some spaces
4 participants
@phadej @piyush-kurur @quasicomputational @hvr