-
-
Notifications
You must be signed in to change notification settings - Fork 100
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
[CCK-4] Add plain text files to build process of artifacts #56
Conversation
cc_licenses/templates/includes/view_legal_code_link_plain_text.html
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.
I'm really impressed with how well this is working. I suggested some improvements.
.travis.yml
Outdated
before-install: | ||
- sudo apt-get -y install pandoc | ||
|
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 looks like Travis has a specific section of the config file that can be used to tell them to add apt packages - https://docs.travis-ci.com/user/installing-dependencies/#adding-apt-packages - it might be better to use that than relying on apt-get.
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
.travis.yml
Outdated
@@ -18,6 +18,9 @@ cache: | |||
env: | |||
- DJANGO_SETTINGS_MODULE="cc_licenses.settings.dev" | |||
|
|||
before-install: | |||
- sudo apt-get -y install pandoc |
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.
Can you add pandoc and the version we've tested with to README.rst in the list of dependencies?
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
|
||
<script> | ||
document.addEventListener("DOMContentLoaded", function() { | ||
const downloadLink = document.querySelector("a#view-plain-text") | ||
path = "{{ request.get_full_path }}" | ||
if (path.endsWith("legalcode")) { | ||
downloadLink.href = "{{ request.get_full_path}}/index.txt" | ||
} | ||
}) | ||
</script> |
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'd be nice if this worked without needing JavaScript. Can we figure out what the plain text link should be in the view and pass it to the template in the context?
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
) | ||
self.assertEqual(200, rsp.status_code) | ||
self.assertGreater(len(rsp.content.decode()), 0) | ||
|
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.
Once we've added the download link to the template context, we might want to add something to the license view tests to check that it's there and correct.
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
licenses/views.py
Outdated
) | ||
if not is_plain_text: | ||
return render(request, **kwargs) | ||
response = HttpResponse(content_type='text/plain; charset="utf-8";') |
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.
Can you remove the last semicolon? https://www.w3.org/International/articles/http-charset/index
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
README.rst
Outdated
(cc_licenses)$ brew install [email protected] | ||
|
||
or for windows via chocolatey: | ||
|
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 homebrew is for Mac, not Linux. But it might be simpler to just link to the install instructions at https://pandoc.org/installing.html rather than trying to give instructions here.
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 and thanks. I should have written apt-get
I believe.
@@ -0,0 +1,331 @@ | |||
{% extends 'base.html' %} |
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.
Do we need a separate template for the HTML for plain text? It looks almost identical to the regular 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.
I think so if we want to eventually better support rtl/ltr languages. If we want drop this as a requirement for now. I think we can go back to the originally implementation where we use beautifulsoup4 to parse hml = render_to_string(**kwargs)
. #56 (comment)
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.
what do you think? We also will have to add back id=plain-text-marker
to each of the templates we are using for licenses.
licenses/views.py
Outdated
template_name="legalcode_40_page.html" | ||
if not is_plain_text | ||
else "legalcode_40_plain_text.html", |
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.
Maybe it's just me, but I always find it just a little harder to read and understand a negated condition (e.g. if not ...
) and try to rewrite it without the not
if there's not some good reason that if not
is better in that place. Could you look at the if not
s in this PR and see if you think some of them would be a little clearer rewritten without the not
?
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.
yes, done. reworked the if statements
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.
Just a couple more comments.
licenses/urls.py
Outdated
@@ -152,6 +152,24 @@ def distill_wireframes(): | |||
name="licenses_default_jurisdiction_and_language", | |||
kwargs=dict(language_code=DEFAULT_LANGUAGE_CODE, jurisdiction=""), | |||
), | |||
path( | |||
# Jurisdiction empty: | |||
# e.g. /licenses/by/4.0/legalcode.es - license BY 4.0 Spanish |
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.
Can you update the example URL in the comment here and below so they look like the URLs that these path()s define?
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.
yes thanks for catching this
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
licenses/views.py
Outdated
"link_to_plain_text_file": f"{ legalcode.license_url() }/index.txt" | ||
if legalcode.license_url().endswith("legalcode") | ||
else f"{ legalcode.license_url() }.txt", |
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 we just define a plaintext_url()
method on the LegalCode class that returns the URL we need, like the license_url()
and deed_url()
methods?
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
Thanks @Audiosutras for making this possible and thanks @dpoirier for your contributions to the PR! Sorry for my ignorance, but I couldn't found the view as plain text button on https://creativecommons.org. Could you point me into the right direction? |
Sorry @santisoler if it's not clear. This repository is still a work in progress and has not been deployed to creativecommons.org. |
Thanks for the response! I suspected that the latest changes weren't deployed yet, but wasn't sure. I'll wait then for the website to get updated. Meanwhile I'll try to locally serve the version on |
Hi @dpoirier! Has the code from this PR been deployed to the CC website? Can we grab translated Thanks again for addressing my issue! |
@santisoler No, this program (and the accompanying data in creativecommons/cc-licenses-data has not yet been brought live / deployed to production). We hope to deploy it soon: 2021 Q2 or Q3. Furthermore, I do not expect the initial deployment to include generated plaintext files (initial deployment will not have translated plaintext files). The plaintext files must be generated in a completely deterministic way as they are used by projects like licensee/licensee for programmatic license identification. Further development of plaintext file generation is currently blocked by Jinja2 Templates · Issue #102 · creativecommons/cc-licenses. For reference, the currently supported plaintext files are linked from: |
Thanks @TimidRobot for the response!
I hope to see that release this year then! 🚀
I see. So I think I'll keep maintaining my https://github.com/santisoler/cc-licenses repo then. For some reason it caught attention and it's getting a lot of contributions, like adding other CC licenses I haven't added in the first place and even translations to multiple languages. It's not like we are making a big change, but it seems that people need a simple way to add a plain text
Thanks for those links, I actually used them for gathering the English licenses in plain text. Thanks again for your work and for the quick response! |
It's difficult to predict future software development, but I hope I'm able to add both plaintext and markdown file generation for all 4.0 license translations later this year. I will also be thinking about how we can organize the documentation so that using CC Licenses in GitHub/GitLab/etc. repositories can be highlighted and support multiple languages. All that to say I really appreciate the work you and the other contributors have done! |
@santisoler I created an issue for any further discussion: Improve support of CC Licensed GitHub repositories · Issue #120 |
Fixes
Fixes #8 by @santisoler
@TimidRobot
Description
This PR adds the creation of
.txt
files for licenses' legal code to our build process. By clickingview legal code as plain text
link users will download the plain text version of the legal code.Technical details
Going forward pandoc will need to be installed.
Please click on the link above for more information on installing pandoc.
Linux:
Windows:
Checklist
Update index.md
).main
ormaster
).visible errors.
Developer Certificate of Origin
Developer Certificate of Origin