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

more liberal white space parsing in the the mixin field #5293

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@piyush-kurur

piyush-kurur commented Apr 27, 2018

This is not yet complete. It is sent for review.

  • 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.

I will add the test cases when I get a go ahead on the commits so far.

@piyush-kurur

This comment has been minimized.

piyush-kurur commented Apr 28, 2018

The travis builds are failing left right and center. However, I do not think it is this commit that is responsible for it. Noticed that there are some further commits on the master branch which is supposed to fix some of the travis builds. Please feel free to give feedback on what is the best course of action.

@piyush-kurur

This comment has been minimized.

piyush-kurur commented Apr 28, 2018

The missing piece is to make cabal-install executable realise that it can now handle CabalSpec = 3.0. After chat with @phadej on IRC, I came to understand that this version is taken from the version of Cabal package. However, a mere bumping up of Cabal version and cabal-install versions would not work as there is the package hackage-security which I guess has an upperbound on Cabal version. That too is needed to be updated.

@23Skidoo

This comment has been minimized.

Member

23Skidoo commented Apr 28, 2018

Hmm, master itself looks fine on Travis, except for the T3436/8.4.2 issue. Try rebasing?

@ezyang

This comment has been minimized.

Contributor

ezyang commented Apr 29, 2018

@piyush-kurur, thanks a lot for taking this on. I also had the "easy" version of this patch languishing on my local copy, but it's a bit more work getting all of the CabalSpec in order. Very happy to see it being done.

@@ -115,18 +135,21 @@ instance Text ModuleRenaming where
parse = do fmap ModuleRenaming parseRns
<++ parseHidingRenaming
<++ return DefaultRenaming
where parseRns = do
rns <- Parse.between (Parse.char '(') (Parse.char ')') parseList
where lparen = Parse.char '(' >> Parse.skipSpaces

This comment has been minimized.

@phadej

phadej Apr 29, 2018

Collaborator

quick comment: don't touch parse = at all. It's hopefully not used anywhere, and better to keep it as 2.0 version. (The more incentives to upgrade to parsec variant there are, the better)

This comment has been minimized.

@piyush-kurur

piyush-kurur Apr 29, 2018

Okey. I will remove it in further commits.

@piyush-kurur

This comment has been minimized.

piyush-kurur commented Apr 29, 2018

@23Skidoo Do you recommend rebasing? I have found github pull requests that are rebased to be quite confusing.

@@ -87,18 +104,21 @@ instance Parsec ModuleRenaming where
-- NB: try not necessary as the first token is obvious
parsec = P.choice [ parseRename, parseHiding, return DefaultRenaming ]
where
lparen = P.char '(' >> liberalSpaces
rparen = P.char ')' >> liberalSpaces

This comment has been minimized.

@piyush-kurur

piyush-kurur Apr 29, 2018

@phadej The liberalSpaces parser just checks for appropriate cabal version and then skips spaces. For lower versions of the cabal spec it says gives an error if there is space. So far so good but I noticed that the spaces stuff was not needed in the version 2.2 case for rparen as there is a P.spaces after its call. Further down the code in the hiding clause there is no P.spaces used which means that the rparen there does not skip spaces (in the version 2.2 behaviour)

If we want to maintain complete bug compatibility, then this will lead to a rather convoluted rparen behaviour. I feel this will eventually lead to more confusing whitespace behaviour in the end and the code will be pepperd with
various flavors of paren-parsers (some that skips spaces and some that do not).

This comment has been minimized.

@piyush-kurur

piyush-kurur Apr 29, 2018

The question therefore is shall I replace the rparen definition with

rparen = P.char ')' >> P.spaces

This is not really compatible with V2.2 I think due to the use of rparen in the hiding clause.

This comment has been minimized.

@piyush-kurur

piyush-kurur Apr 29, 2018

I just realised that this has implication to some of the tests that failed. So let me send a commit with the above version of rparen. If you find it not okey I will revert it again.

@ezyang

This comment has been minimized.

Contributor

ezyang commented Apr 30, 2018

@piyush-kurur If you haven't yet decided on a way to structure the old and new parsers, I'd recommend just doing a straight up copy paste of the entire grammar and having two copies.

@piyush-kurur

This comment has been minimized.

piyush-kurur commented Apr 30, 2018

@ezyang What you say makes sense. I will make the necessary changes and get back.

@piyush-kurur

This comment has been minimized.

piyush-kurur commented May 1, 2018

@ezyang I have kind of done what you have asked me. There are two questions that I want to make

  1. Is it okey for me to merge the last three commits (ebb6a67, 9df8823, and 95fd2a9) into one commit via a rebase -i. This might make the commit more readable and it would be clear what changed ?

  2. There is one problem still which I have added as a TODO in the file. If a user declares cabal-version:2.2 and uses a more liberal spaces, then this version will just throw an error like the old version. However, we might want to give a helpful suggestion here. I cannot see a way to have this incorporated cleanly.

@ezyang

This comment has been minimized.

Contributor

ezyang commented May 3, 2018

LGTM, though @phadej should probably sign off on the BC bits (I don't quite understand the correctness criterion here.) Happy to merge it in a few days if radio silence.

@ezyang

This comment has been minimized.

Contributor

ezyang commented May 3, 2018

(and yes, I would squash)

@piyush-kurur

This comment has been minimized.

piyush-kurur commented May 3, 2018

@ezyang Okey I will do the squash.

Also @phadej please hold on merging this change as there is one piece still missing. The cabal-install executable should be made to realise that it can now handle V3 specs. As I remarked in the previous comment, this requires changing the version in the .cabal files of Cabal, cabal-install and I think also hackage-security. Is that all that is required ?

@piyush-kurur piyush-kurur force-pushed the piyush-kurur:whitespace-issue branch from ebb6a67 to 05105c2 May 3, 2018

@piyush-kurur

This comment has been minimized.

piyush-kurur commented May 3, 2018

@ezyang done the squash

@hvr

This comment has been minimized.

Member

hvr commented May 3, 2018

As I remarked in the previous comment, this requires changing the version in the .cabal files of Cabal

not necessarily; we just need to infer (or hard-code) a "rounded up" value as the current spec version; something we should do anyway as it would simplify our workflow.

@piyush-kurur

This comment has been minimized.

piyush-kurur commented May 3, 2018

@hvr. Do let me know then how to proceed from here. Is there any plans to sync Cabal (the file format) specs with Cabal (the library version) ?

@piyush-kurur

This comment has been minimized.

piyush-kurur commented May 3, 2018

Also test case. I have a failing case that I have mentioned in #5292. Let me know how this can be added to the testsuite.

@23Skidoo

This comment has been minimized.

Member

23Skidoo commented May 9, 2018

@phadej, can you please take a look at this?

@piyush-kurur

This comment has been minimized.

piyush-kurur commented May 26, 2018

any updates on this?

danidiaz pushed a commit to danidiaz/cabal that referenced this pull request Jun 18, 2018

daniel
Added note to the documentation of "mixins:"
It mentions the issue of parsing failures caused by the presence of
blank spaces after opening parentheses.

See haskell#5150, haskell#5292, haskell#5293.

23Skidoo added a commit that referenced this pull request Jun 19, 2018

Added note to the documentation of "mixins:"
It mentions the issue of parsing failures caused by the presence of
blank spaces after opening parentheses.

See #5150, #5292, #5293.
@23Skidoo

This comment has been minimized.

Member

23Skidoo commented Jun 20, 2018

@phadej, can you please take a look?

@quasicomputational

This comment has been minimized.

Collaborator

quasicomputational commented Aug 7, 2018

There's a specific mention of this in the docs that will need to be removed once this is merged.

@phadej phadej referenced this pull request Aug 11, 2018

Open

Piyush whitespace issue #5514

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