-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
License badge colour corresponding to license type #1190
Conversation
List of all licenses (spdx_id, name). I have to categorize them and put in config file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
lib/licenses.js
Outdated
acc[licenseName] = licenses[licenseType].color; | ||
return acc; | ||
}, licenseToColor); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you export a function instead? It makes it a bit easier to handle errors, and you could also use it to handle default values, as in some of the functions in lib/badge-data.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've exported licenseToColor as a function.
lib/licenses.js
Outdated
licenses[licenseType].licenses.reduce((acc, licenseName) => { | ||
acc[licenseName] = licenses[licenseType].color; | ||
return acc; | ||
}, licenseToColor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be clearer?
Object.keys(licenseTypes).forEach(licenseType => {
const { licenses, color } = licenseTypes[licenseType];
licenses.forEach(license => {
licenseToColor[license] = color;
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be :-) I've change the code.
lib/licenses.spec.js
Outdated
|
||
describe('licenseToColor', function () { | ||
it('should has entry for known license', function () { | ||
assert.equal(licenseToColor['MIT'], 'blue'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/licenses.js
Outdated
'use strict'; | ||
const licenses = { | ||
'permissive': { | ||
licenses: ['MIT', 'Apache-2.0', 'BSD-3-Clause', 'BSD-2-Clause'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these SPDX IDs or are they github-specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are SPDX ids. Rename licenses
to spxdLicenseIds
?
I noticed this is only change one of the many license badges. Any thoughts about whether any of the others should be updated? |
I guess you can use tldrlegal API and check |
I've added all licenses supported by GitHub to |
@KOLANICH so you want to get license details from tldrlegal API in order to categorize them into one of the three groups (permissive, viral and public domain)? I thought config like |
You will have to update the config manually when new licences appear. I think it's better to have a script rebuilding the "config" (in fact it is used as a database) automatically on schedule. |
@paulmelnikow I haven't thought about other license badges (we were talking in issue about licenses recognized by GitHub). But there is a lot of other license badges and they should be updates as well. |
On the one hand it is a good idea to load licenses details from. On the other hand this 31 licenses from Github could cover most of the licenses (compare with some licenses distribution stats https://www.whitesourcesoftware.com/whitesource-blog/open-source-software-licenses-trends/, https://www.blackducksoftware.com/top-open-source-licenses).
|
|
@KOLANICH if you're passionate about automatically generating the license data, how about publishing an npm package with that data in it? Then we could rework this to use that data. I'm not sure tldrlegal is the best source of data to use. That API is "temporary", and you may need their permission to publish their data. I found this folder of data from Github's choosealicense.com project, which I think is under CC-BY. Maybe that would be a good source. |
@KOLANICH Open Software License 3.0 (OSL-3.0) in tldrlegal.com does not have |
Thanks for collecting those license badges. The only one of those registries that I know uses SPDX IDs is npm. It's worth enabling this code for that badge. They do support SPDX expressions which I just used for the first time. I don't encounter them very often though. You could write some code to handle them, or just display in gray as unrecognized. License string parsing is a can of worms! I'm game to attempt it but would probably want to take that seriously, collecting lots of example strings from each repository and making sure we're doing a good job detecting them + labeling them correctly. I'm not sure it feels worth it for just the color, though do think a "convert license to SPDX" could be generally useful. Maybe we could even convince some of these repositories – or projects – to publish the SPDX id! Well, that's a grand vision… if you want to work on that I'd be game to help, though probably we should make a separate repo. |
lib/licenses.js
Outdated
@@ -0,0 +1,29 @@ | |||
'use strict'; | |||
const licenseTypes = { | |||
'permissive': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain how these were derived from https://choosealicense.com/appendix/
, and give attribution? I think that content is licensed CC-BY. It would also make it easier to check your work, as you requested!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'public-domain' licenses are those that do not require 'license and copyright notice', 'viral' - requires 'disclose source' or 'same license', rest is 'permissive'. Does it answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, great! Could you add this in a comment? I feel like that'll be helpful to others in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I'm not going to do that. I don't use node and npm and I don't understand and strictly against of all this hype of using npm even for projects not involving Node, such as webpages, libraries not specific to node and extensions for browsers.
Neither do I.
It has been "temporary" for more than 4 years. We in Russia have a proverb "there is nothing more permanent than temporary one".
It is the main problem here. I guess you should contact them and ask. Since there are links to json from the pages of licenses on tldrlegal (in the bottom), I guess that the chances are high to get the permission.
That should be a good source. I guess we need a separate repo with json files.
Then tldrlegal doesn't suit our needs well, for now. |
I was thinking a bit more about the meaning behind the colors. Some people love copyleft licenses and think they are the best. Others see them and think, danger! Ideally, we should choose a color for copyleft that works for both schools of thought, conveying both "awesome!" and "danger!". Since we're asking people to put these badges on their own projects, it's important people can relate to the color. (Otherwise they will probably just override it.) Also, since many projects have license badges with our blue color, it might be better to choose something else for permissive. What do you think about these? |
Orange looks nice, but gray for unknown doesn't, because unknown may be proprietary, so unknown is danger, and more danger than viral. And explicitly or implicitly (missing license) proprietary is more dangerous than unknown.
In fact most of templates of licence shields are blue. I guess it is because it is one of the colors having no success/failure meaning. In fact I thought about something like |
@platan I would love to get this merged. When you have a moment, could you take a look at my last round of comments? |
I will continue work on this next week. |
Could you merge in master? Would love to get this landed! Thanks for your patience. |
service-tests/github.js
Outdated
|
||
t.create('Public domain license') | ||
.get('/license/badges/shields.json?style=_shields_test') | ||
.expectJSON({ name: 'license', value: 'CC0-1.0', colorB: '#7cd958' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since licenseToColor
has its own unit tests, how about doing something like:
const permissiveLicenseColor = licenseToColor('CC0-1.0');
That way if we change the color, this test won't require new maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is pending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I had to stop work on this suddenly. Thanks!
given({}, 'f00f00').expect({ colorB: '#f00f00' }); | ||
given({ colorB: '#f00f00', colorscheme: 'blue' }, 'red').expect({ colorscheme: 'red' }); | ||
given({ colorB: '#b00b00', colorscheme: 'blue' }, 'f00f00').expect({ colorB: '#f00f00' }); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
service-tests/github.js
Outdated
|
||
t.create('Public domain license') | ||
.get('/license/badges/shields.json?style=_shields_test') | ||
.expectJSON({ name: 'license', value: 'CC0-1.0', colorB: '#7cd958' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is pending.
I ran the service tests locally. There are a slough of failures, though most of them are unrelated. There are related failures in the GitHub service tests, which are returning license | missing. Could you take a look? |
You probably exceeded GitHub API rate limit. In this case you get "missing" as a license value. I've changed this to "inaccessible" (for this and other cases when HTTP status code is not 200). I've added a service test for this case as well. edit: |
Huh, I'd be surprised if it were rate limit, since I have a token set up in development (5k/hr). Nevertheless, they are passing now! And thanks for the improved erroring! |
I was a really long PR :-). @KOLANICH @paulmelnikow and @tooomm thanks for ideas, reviews and your time! |
👏 🎉 |
It was. Well done @platan! And thanks... |
Yes, I was thinking the same thing. Well done! Looking forward to getting this shipped so we can start hearing feedback about it. Was thinking we might want to document an explanation of the colors somewhere on the website. Could be in an example, or else at the bottom of the page. Should be live on staging, at any rate: https://shields-staging.herokuapp.com/ |
Indeed, some documentation would be very helpful here! I would prefer it inside every license badge preview it applies to. Edit: Actually there is some more heavy documentation within the badge preview for |
Implementation of #1093.
Listing of all licenses (https://developer.github.com/v3/licenses/#list-all-licenses) returns 12 licenses:
but when we use advances search https://github.com/search/advanced there is more of them
API returns info about this extra license:
I do not have service tests for colours, cause json resource does not return colour :-( Should we add colour to json resource in order to test them?
TODO:
viral
tocopyleft
copyleft
andviral
licensesunknown
label for GitHubOther
licensedefault
)