Fields parser #4702

phadej · 2017-08-16T23:33:58Z

this is PoC of approach to parse fields

The difference is visible in FieldDescr.hs and Cabal/Distribution/PackageDescription/Parsec.hs

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.

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

mention-bot · 2017-08-16T23:33:59Z

@phadej, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ezyang, @23Skidoo and @Ericson2314 to be potential reviewers.

phadej · 2017-08-16T23:34:33Z

Cabal/Distribution/PackageDescription/Parsec/FieldDescr.hs

-
-sourceRepoFieldDescrs :: [FieldDescr SourceRepo]
-sourceRepoFieldDescrs =
-    [ simpleField "type"


I really don't like this boilerplate

phadej · 2017-08-16T23:35:16Z

Cabal/Distribution/PackageDescription/Parsec.hs

+                <*> affineFieldAla "module"   Token      repoModule
+                <*> affineFieldAla "branch"   Token      repoBranch
+                <*> affineFieldAla "tag"      Token      repoTag
+                <*> affineFieldAla "subdir"   FilePathNT repoSubdir


the applicative value will live somewhere else, as we'll need it for pretty-printer

phadej · 2017-08-16T23:35:41Z

Cabal/Distribution/Parsec/Types/FieldParser.hs

+uniqueField = error "define me"
+
+-- | Field which can be defined at most once.
+affineField


I'll rename to optionalField

phadej · 2017-08-16T23:36:04Z

Cabal/Distribution/Parsec/Types/FieldParser.hs

+    parser fields = case Map.lookup fn fields of
+        Nothing -> pure Nothing
+        Just [] -> pure Nothing
+        Just [MkNamelessField pos fls] -> Just . (unpack :: b -> a) <$> runFieldParser pos parsec fls


todo: if many fields, take last.

phadej · 2017-08-16T23:36:26Z

Cabal/Distribution/Parsec/Types/FieldParser.hs

+-- Newtype
+-------------------------------------------------------------------------------
+
+class Newtype n o | n -> o where


http://hackage.haskell.org/package/newtype-0.2/docs/Control-Newtype.html

phadej · 2017-08-16T23:37:30Z

The core point is that "if you add a field, FieldFancy will break".

phadej · 2017-08-16T23:39:38Z

Cabal/Distribution/PackageDescription/Parsec.hs

@@ -297,7 +300,18 @@ parseGenericPackageDescription' lexWarnings fs = do
                _ -> do
                    parseFailure pos $ "Invalid source-repository kind " ++ show args
                    pure RepoHead
-            sr <- parseFields sourceRepoFieldDescrs warnUnrec (emptySourceRepo kind) fields


also note that one won't need fishy emptySourceRepo anymore, and similars for other types!

phadej · 2017-08-16T23:44:18Z

Cabal/Distribution/Parsec/Types/FieldParser.hs

+    pack = FilePathNT
+    unpack = getFilePathNT
+
+instance Text FilePathNT


Text instances will be replaced with Display class (I once tried to add, but I have to try again!)

ezyang · 2017-08-17T01:43:31Z

Cabal/Distribution/Compat/Newtype.hs

+-- functions such as 'ala'.
+module Distribution.Compat.Newtype where
+
+-- TODO: export only Newtype (..), ala


Yes please!

ezyang · 2017-08-17T01:44:00Z

Cabal/Distribution/Compat/Newtype.hs

+ala' _ hof f = unpack . hof (pack . f)
+
+-------------------------------------------------------------------------------
+-- Move to own module


Yes please.

phadej · 2017-08-17T13:20:35Z

Cabal/Distribution/PackageDescription/Parsec/FieldDescr.hs

+    <*> optionalFieldAla "module"   Token      repoModule
+    <*> optionalFieldAla "branch"   Token      repoBranch
+    <*> optionalFieldAla "tag"      Token      repoTag
+    <*> optionalFieldAla "subdir"   FilePathNT repoSubdir


this patch thunk is the reason for new functionality.

phadej · 2017-09-05T09:03:28Z

This PR is ready. I'll write a changelog stub separately.

hvr

This is a big patch, and tbh, it had a bit of noise (import-list & pragma-list normalisations) that made it more difficult to focus on what matters.

Btw, now I understand why you'd want a parser abstraction along the lines of http://hackage.haskell.org/package/parsers...

However, I couldn't see anything obviously bad, so this looks good for me. Afaik, @23Skidoo wanted to give this PR a review as well.

23Skidoo · 2017-09-09T12:58:00Z

it had a bit of noise (import-list & pragma-list normalisations)

I usually just ignore this stuff. If something's wrong with it, CI will tell you.

phadej · 2017-09-14T05:10:32Z

@23Skidoo are you planning to review this, or should I merge?

23Skidoo · 2017-09-14T05:33:16Z

@phadej Sorry for the delay, I'll try to finish the review today.

23Skidoo · 2017-09-19T15:58:23Z

Cabal/Distribution/FieldGrammar/Parsec.hs

+-- Fields can be specified multiple times in the .cabal files.  The order of
+-- such entries is important, but the mutual ordering of different fields is
+-- not.Also conditional sections are considered after non-conditional data.
+-- The example of this silent-commutation quirck is the fact that


s/quirck/quirk/

23Skidoo

Amazing job! LGTM modulo minor comments. Sorry for procrastinating with the review.

23Skidoo · 2017-09-19T16:12:19Z

Cabal/Cabal.cabal

@@ -274,18 +277,22 @@ library
    parsec >= 3.1.9 && <3.2
  exposed-modules:
    Distribution.Compat.Parsec
+    Distribution.FieldGrammar


Hmm, can this stuff live under e.g. Distribution.Parsec.FieldGrammar instead?

It's both related to Parsing and Pretty-printing, and actually will be a little related to command line parsing too.

23Skidoo · 2017-09-19T16:21:11Z

Cabal/tests/ParserTests.hs

+    x <- parse contents
+    let contents' = showGenericPackageDescription x
+    y <- parse (toUTF8BS contents')
+    let y' = if x ^. L.packageDescription . L.license == UnspecifiedLicense


Can you please add a comment here explaining why special-casing of UnknownLicense "UnspecifiedLicense" is needed here and whether we can make it go away in the future?

It will hopefully go away in SPDX change. The short explanation is that parse . pretty roundtrip on License doesn't hold ATM. Yet Hackage never accepts a package with UnspecifiedLicense, so I didn't bother to refactor it yet.

23Skidoo · 2017-09-19T16:42:13Z

Cabal/tests/ParserHackageTests.hs

+    y0 <- parse "2nd" (toUTF8BS bs')
+
+    -- unspecified license
+    let y1 = if x0 ^. L.packageDescription . L.license == UnspecifiedLicense


There is some duplication here with the ParserTests module. Maybe worth consolidating various data scrubbing utilities used in the parser test suite in a single place somewhere?

I'll think about license stuff particularly when doing SPDX change, but yeah, there are commonalities. I'll factor them out in separate PR.

23Skidoo · 2017-09-19T17:13:45Z

Cabal/Distribution/Pretty.hs


 showToken :: String -> PP.Doc
 showToken str
+    | "--" `isPrefixOf` str                 = PP.text (show str)


Would be nice with a comment here.

I'll add a comment, but --foo looks like a comment.

23Skidoo · 2017-09-19T17:31:45Z

Cabal/Distribution/FieldGrammar.hs

+(^^^) :: a -> (a -> b) -> b
+x ^^^ f = f x
+
+-- | Partitionin state


s/Partititionin/Partitioning/.

phadej · 2017-09-19T21:19:27Z

I'll fix typos and add comments tomorrow. If there is no objection I'll merge this after that.

This commit reworks how GenericPackageDescription is parsed from `[Field Position]` and pretty-printed to `Doc`. This also fixes few issues: - Fix haskell#4697: `cabal format` doesn't output custom-setup stanza (nor foreign-lib stanzas) - Fix haskell#4719: `parse . pretty . parse = parse` for all Hackage cabal files. - `parser-hackage-tests roundtrip` is the test program. The handling of `license-file` and `license-files` is changed. Now they behave the same.

phadej requested review from 23Skidoo and hvr August 16, 2017 23:33

phadej commented Aug 16, 2017

View reviewed changes

phadej force-pushed the fields-parser branch from 861bdb2 to a52091a Compare August 17, 2017 00:15

ezyang reviewed Aug 17, 2017

View reviewed changes

phadej force-pushed the fields-parser branch 3 times, most recently from f2dc34a to 73304e6 Compare August 17, 2017 13:19

phadej commented Aug 17, 2017

View reviewed changes

phadej force-pushed the fields-parser branch 16 times, most recently from f1a7752 to 60e0f79 Compare August 20, 2017 15:42

phadej force-pushed the fields-parser branch 14 times, most recently from 690a725 to 4d6057b Compare September 5, 2017 07:40

phadej force-pushed the fields-parser branch from 4d6057b to 80c19b4 Compare September 5, 2017 15:43

phadej mentioned this pull request Sep 8, 2017

Elif #4750

Merged

3 tasks

hvr approved these changes Sep 9, 2017

View reviewed changes

23Skidoo reviewed Sep 19, 2017

View reviewed changes

23Skidoo approved these changes Sep 19, 2017

View reviewed changes

phadej added 2 commits September 20, 2017 10:56

Fix typos and add comments

66e2232

phadej force-pushed the fields-parser branch from 80c19b4 to 66e2232 Compare September 20, 2017 08:02

phadej merged commit 1e1ed45 into haskell:master Sep 20, 2017

phadej deleted the fields-parser branch September 20, 2017 16:23

phadej mentioned this pull request Nov 4, 2017

showGenericPackageDescription leaves out many fields of executable stanza #4863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fields parser #4702

Fields parser #4702

phadej commented Aug 16, 2017 •

edited

Loading

mention-bot commented Aug 16, 2017

phadej Aug 16, 2017

phadej Aug 16, 2017

phadej Aug 16, 2017

phadej Aug 16, 2017

phadej Aug 16, 2017

phadej commented Aug 16, 2017

phadej Aug 16, 2017 •

edited

Loading

phadej Aug 16, 2017

ezyang Aug 17, 2017

ezyang Aug 17, 2017

phadej Aug 17, 2017

phadej commented Sep 5, 2017

hvr left a comment

23Skidoo commented Sep 9, 2017

phadej commented Sep 14, 2017

23Skidoo commented Sep 14, 2017

23Skidoo Sep 19, 2017

23Skidoo left a comment

23Skidoo Sep 19, 2017

phadej Sep 19, 2017

23Skidoo Sep 19, 2017

phadej Sep 19, 2017

23Skidoo Sep 19, 2017

phadej Sep 19, 2017

23Skidoo Sep 19, 2017

phadej Sep 19, 2017

23Skidoo Sep 19, 2017

phadej commented Sep 19, 2017

Fields parser #4702

Fields parser #4702

Conversation

phadej commented Aug 16, 2017 • edited Loading

mention-bot commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phadej commented Aug 16, 2017

phadej Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phadej commented Sep 5, 2017

hvr left a comment

Choose a reason for hiding this comment

23Skidoo commented Sep 9, 2017

phadej commented Sep 14, 2017

23Skidoo commented Sep 14, 2017

Choose a reason for hiding this comment

23Skidoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phadej commented Sep 19, 2017

phadej commented Aug 16, 2017 •

edited

Loading

phadej Aug 16, 2017 •

edited

Loading