Skip to content

Introduce new license-files field. #1048

Closed
wants to merge 4 commits into from

4 participants

@mboes
mboes commented Sep 23, 2012

Hi, this is a followup pull request to "New optional field: extra-license-files". This patch also introduces the ability to have multiple license files, though following feedback, unlike the previous pull request, this one uses the following approach, quoted from the commit message:

The existing license-file field is an alias to license-files,
restricted to only allow one license file to be specified (as is the
common case). Allowing multiple license files is necessary for some
licenses, such as the LGPL.

I myself prefer the extra-license-files option, but feel free to merge whichever pull request and close the other one.

@mboes mboes Introduce new license-files field.
The existing license-file field is an alias to license-files,
restricted to only allow one license file to be specified (as is the
common case). Allowing multiple license files is necessary for some
licenses, such as the LGPL.
39cbff6
@dcoutts
Haskell member
dcoutts commented Nov 1, 2012

Generally looks good. A couple comments:

You've removed the extra-source-files field in the parser. I don't think you really meant to do that right?

Secondly, in the field parser, note that it actually doubles as a pretty printer. I've not actually tested it, but the (head . licenseFiles) bit looks suspicious. I have two worries there: one that we'll get head [] if the list is empty but also that when we pretty print we'll likely end up printing both fields license-file and license-files, when we'd only want to print one, depending on if there's one or more files. Can you try out the pretty printing and see what happens in each case (0 files, 1 file, 2+ files). If it's not doing the right thing, ask and we can help.

@tibbe
Haskell member
tibbe commented Mar 15, 2013

@mboes Do you mind looking into @dcoutts comments?

mboes added some commits May 4, 2013
@mboes mboes Don't touch extra-source-files in Parse.hs.
That was a typo.
125e343
@mboes mboes Introduce an alternation combinator for UnrecParser, called 'composeU…
…nrec'.
85da14d
@mboes mboes Have only a single 'license-files' field.
A previous patch introduced license-file field as an alias to
license-files, but this has the downside that the pretty printer will
print both. Instead, we now only have the license-files field and use
the UnrecParser mechanism to parse the license-file field.
4ca0a39
@mboes
mboes commented May 4, 2013

@tibbe @dcoutts sorry for the lag - wasn't getting any notifications. I have made some updates to to this pull request to address the concerns of @dcoutts. The current codebase is ill suited to introduce field aliases. So to avoid pretty printing both license-files and license-file, I removed license-file, introduced license-files instead, and I introduced an UnrecParser to parse license-file.

TBH I am now all the more convinced that the right approach here is to just add a new field for extra license files, as was the case in my first pull request #970 (which has since been closed). See my comment at the end of that pull request as to the rationale. I now think keeping license-file and introducing extra-license-files can be done with more readable code than making license-file an alias to license-files.

As such, I recommend merging #970 rather than this pull request.

@mboes
mboes commented Oct 17, 2013

@dcoutts @tibbe There hasn't been progress on this merge request in a while. As explained in my last comment, I would prefer #970 to be merged rather than this one. But #970 is closed now. Could that one be reopened, merged, and then we can close this one?

@23Skidoo
Haskell member

@mboes

I've reopened #970.

@tibbe
Haskell member
tibbe commented Dec 19, 2013

Closed in favor of #970.

@tibbe tibbe closed this Dec 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.