-
Notifications
You must be signed in to change notification settings - Fork 215
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
Update openverse-attribution
with new features and improvements
#4250
Conversation
packages/python/openverse-attribution/src/openverse_attribution/license_name.py
Outdated
Show resolved
Hide resolved
packages/python/openverse-attribution/src/openverse_attribution/license_name.py
Outdated
Show resolved
Hide resolved
packages/python/openverse-attribution/src/openverse_attribution/data/all_licenses.json
Show resolved
Hide resolved
packages/python/openverse-attribution/src/openverse_attribution/license.py
Show resolved
Hide resolved
packages/python/openverse-attribution/src/openverse_attribution/license.py
Outdated
Show resolved
Hide resolved
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.
LGTM. I think there are opportunities to make this library more flexible and if we got to the point of publishing it, I think we should. But you're right, this package as it is, and maybe for forever, is meant to share logic across parts of Openverse codebase, not solve a general problem.
Sorry I got sidetracked with that. LGTM!
💯 We should definitely look into making this more generalised and also consider open sourcing it. But until that time, we should take up smaller incremental improvements like this one. |
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.
So many comprehensive changes ⭐
In the catalog, we have a function to get the license information based on the license name-license version pair, and/or the licence URL. I think we should add the ability to get License
from a URL, without providing the license name-version pair. It should probably done as a separate PR, though.
packages/python/openverse-attribution/src/openverse_attribution/license.py
Outdated
Show resolved
Hide resolved
:param version: the version number of the license | ||
:return: the full name of the license | ||
# Shorten long variable names | ||
ver = version |
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 think it would be easier to read if we didn't shorten the version
. jur
is pretty clear, but ver
, for me, seems more unclear.
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 was enjoying the similarities in both the length (3 letters) and the pronunciations.
packages/python/openverse-attribution/src/openverse_attribution/license.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Olga Bulat <[email protected]>
Co-authored-by: Olga Bulat <[email protected]>
Fixes
Partially relates to #4182.
Description
This PR
openverse-attribution
to add lots of new featuresopenverse-attribution
for a clearer APILicense
enum renamed toLicenseName
.License
wrappingname
(LicenseName
) and optionallyversion
(str
) andjurisdiction
(str
).License
classopenverse-attribution
openverse-attribution
in the API