Skip to content

Conversation

@mblayman
Copy link
Contributor

This branch includes a modification to the release detail page to replace the license TODO with the Release.license attribute.

I removed the hyperlink because license is an unstructured text field from distutils and it's not possible to exactly determine what the URL of that field might be (e.g., the code might be able to guess the correct license URL for "BSD", however, "Project X custom license" would not be feasible to provide a license URL).

The one aspect that I did not know how to handle was the case of a missing license. My thought was to put some default value like UNKNOWN in that spot. I'm happy to do that, but I don't know how to set l20n so that it will know how to deal with a default value.

As this is my first contribution to this project, I'd appreciate any feedback if I'm missing something important. Thanks! 😄

@mblayman
Copy link
Contributor Author

I've been poking around the issues page and found that this fix is probably insufficient (who would have thought that some people would put entire license text into that field!).

I'm still very open to helping make this a useful branch and can work on the necessary changes.

My initial thought that comes to mind is to display only the first line of text of the Release.license field contains multiple lines.

Something else to consider would be favoring an existing license Trove classifier over the Release.license field if the classifier exists. Maybe the display of the classifier would be as the right most segment (e.g., BSD License from License :: OSI Approved :: BSD License).

@dstufft
Copy link
Member

dstufft commented Jun 19, 2016

@mblayman I'm of the mind to just ignore the license field and only use classifiers.. or maybe if there's no classifiers and the license field contains some text, add a link that says "Other" and opens up a modal dialog with the content of the license field or something.

Maybe @nlhkabu has a better idea.

@mblayman
Copy link
Contributor Author

I updated this branch to be smarter than just using the Release.license metadata field. The view follows these rules:

  1. Use the last part of a license classifier, if one exists (e.g., BSD License from License :: OSI Approved :: BSD License).
  2. Use the first line of Release.license (e.g. Bizarre Proprietary License from Bizarre Proprietary License\nFull text beyond here...).
  3. Hide the License display when neither a classifier nor metadata are provided.

@nlhkabu
Copy link
Contributor

nlhkabu commented Jun 20, 2016

Thanks for your work on this @mblayman

On my side I'd much prefer not to have a modal (we don't currently have one, so I'm not keen on integrating one just for this)...

Regarding your solution:

It looks as though if both a classifier and release.license are present, we will show both. I'm not sure that this is the best solution... because we might end up displaying duplicate or contradictory information. @dstufft which piece of data should we consider the 'authority' here?

I'm a bit concerned about the first line strategy yeilding inconsistent results because of inconsistent data. @dstufft might be able to provide more information on what data we should expect here.

@mblayman
Copy link
Contributor Author

@nlhkabu, I tried to write the code in a way that it will take the classifier if it exists. While it does always attempt to take the license metadata first, the presence of a classifier will always override the metadata value.

I'll make sure it does that. Looks like I need to add another test for additional code coverage anyway.

@mblayman
Copy link
Contributor Author

I updated a test case to a) boost coverage and b) show that a classifier is preferred over license metadata. I hope this clears things up.

@dstufft
Copy link
Member

dstufft commented Jun 30, 2016

So we have a couple of open questions here in my mind.

  1. Is the Release.license field reasonably formatted in enough cases to stick it sort of free form in the top of the page where the License information should go?
  2. If we're going to use both Release.license and classifiers, what should we do when something contains both of them?
  3. What if something contains multiple licenses?

Personally for myself I'm thinking:

  1. I think that there's just junk data here to realistically pull meaning information out of it. We have values ranging from a handful of characters to over 32,000 characters in the database. I'm thinking at most we should render this somewhere else (maybe another tab on the side? I don't know) or not at all.
  2. Given my feelings on (1), this question becomes moot.
  3. I think we should include all of the license classifiers, not just the first one.

One idea that's kind of sticking in my head though, is I wonder if it'd make sense to try and translate the classifiers to SPDX License language? Perhaps an even further idea is to repurpose the license field to be defined as a SPDX License Expression and have PyPI validate that, for all new packages, that field contains only valid license expressions. Then we could have the tag at the top be license expressions which for the most common cases will just be short names for each licenses, but can also easily represent more complex cases like "GPLv2 or later", "BSD OR Apache", "BSD AND Apache", or even GPLv2 or later with the Bison 2.2 exception". If we did that we could link each class in the expression to the canonical location for that license and I think it'd end up being real nice.

What do folks think about that?

@mblayman
Copy link
Contributor Author

Those are good questions @dstufft.

  1. I presume that the Release.license is reasonably formatted for some cases. I wrote the code to grab the first line in case the content is not reasonable. I don't believe that any solution using Release.license will display perfectly, however, displaying the first line seemed like a reasonable compromise to me.
  2. As implemented, if both Release.license and a license classifier exist, this branch will always choose the license classifier. That seemed like the right thing to do because the classifiers have a good level of consistency.
  3. I did not account for multiple licenses in this branch so far. I can certainly update the branch to collect license classifiers into a list and then return a joined version. In that case, a packaging containing License :: OSI Approved :: BSD License and License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+) could produce an output of BSD License, GNU General Public License v2 or later (GPLv2+).

While SPDX seems interesting, I suspect that repurposing the existing Release.license field would cause some backwards compatibility trouble. The SPDX list is limited, and if authors are given no option to provide whatever license they choose, it seems like PyPI would place an unnecessary restriction on them. While I personally feel that uploading a package with a non-standard open source license is foolish, I lean toward openness for packages that may exist in unusual circumstances and need unusual licenses.

@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 1, 2016

From a UI perspective, I'm not keen on moving the license to a separate tab - given the content could be just a few words, I think there is a risk it is going to look really odd.

I think we should move license to the bottom of the header (e.g. under the 'pip install box'). This will allow for longer strings - including multiple licenses as @mblayman describes in point 3.

For any licenses that are truncated after the first line, we can add a 'read more' link that launches the full thing in a modal (yes, Donald, I've come around to your thinking on the modal :P ).

For launch I can probably live without the modal being done (with just the truncated text showing)- we should open a separate issue for this.

@mblayman
Copy link
Contributor Author

mblayman commented Jul 2, 2016

This branch is now updated to support multiple license classifiers. I took @nlhkabu's advice and moved the license label so that it is below the pip install <package> portion. I agree with her sentiment of doing the modal and "Read more" work later and tracking it with a separate issue.

@dstufft
Copy link
Member

dstufft commented Jul 2, 2016

They code changes here look reasonable to me, if @nlhkabu is happy with the UI changes (or if she plans on adjusting it herself) this looks fine to merge to me.

@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 2, 2016

@mblayman are you able to provide me with a screenshot so I can verify this visually?

@mblayman
Copy link
Contributor Author

mblayman commented Jul 2, 2016

Sure thing.

license-below

@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 2, 2016

Thanks @mblayman - in the short term, can you please remove the right alignment. If I need to adjust the margins, I can do so when I make other updates to this section :)

@mblayman
Copy link
Contributor Author

mblayman commented Jul 2, 2016

All set.

right-aligned

@nlhkabu nlhkabu merged commit 218a602 into pypi:master Jul 2, 2016
@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 2, 2016

✨ 🎉

@mblayman
Copy link
Contributor Author

mblayman commented Jul 2, 2016

Sweet! Thanks for all the feedback along the way. It feels nice to be able to contribute a small bit back to a community that I've been working in for years.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants