GH-1564: Add "Last Modified" trailer to PEP HTML pages #1565
Conversation
Obtain the last modified date by making GET request to GitHub. Don't render anything if there's any error with the GET request. Add tests, and mock the network call. Closes #1564
|
Instead of getting the last commit date through @berkerpeksag please review when you have the chance. |
|
|
||
| json_data = data.json() | ||
| if len(json_data) > 0: | ||
| commit_date = json_data[0]['commit']['committer']['date'] |
There was a problem hiding this comment.
Are there any failure conditions in which this would generate a KeyError?
There was a problem hiding this comment.
As far as I'm aware, github's API will always return [] upon finding no results or error, and when results are found for the repository's commits, there will always be an accessible commit.committer.date field. So, the KeyError would not occur in any situation (that I'm aware of) where data.json() returns something with a length > 0 when targeted at that specific URL.
|
Wow - fast work, @Mariatta! The issue also discusses the benefits of adding metadata such as |
|
@berkerpeksag will you have time to look into this and review, and perhaps help merge? @nealmcb regarding additional metadata like datePublished or dateModified, I think those will come from the content of PEP file, which we already have access to. It will not come from Sorry for the delay in following up on this. |
Yes, sorry for my late response. Will try to have a look this weekend. |
|
Thanks for working on this Mariatta! Better links to git repo and history page can help people find more information and address the criticism/confusion in the original mail thread (or forum, I forgot). It doesn’t feel great that a local build process, working on local files and with all info in a local git repo, now needs to send many external requests to one repo host. Using git plumbing commands in subprocess wouldn’t be so bad? Or maybe one of the Python git libs could be nicer to use? (I could investigate that next week) |
| try: | ||
| data = requests.get( | ||
| 'https://api.github.com/repos/python/peps/commits?path={}'.format(pep_filename) | ||
| ) | ||
| except Exception: | ||
| return commit_info |
There was a problem hiding this comment.
| try: | |
| data = requests.get( | |
| 'https://api.github.com/repos/python/peps/commits?path={}'.format(pep_filename) | |
| ) | |
| except Exception: | |
| return commit_info | |
| try: | |
| data = requests.get( | |
| 'https://api.github.com/repos/python/peps/commits?path={}'.format(pep_filename) | |
| ) | |
| data.raise_for_status() | |
| except Exception: | |
| return commit_info |
It might be worthwhile to add data.raise_for_status(), to ensure that we always raise an exception here if we get an http error code like 404, 401, etc. Otherwise, it might raise a JSONDecodeError when we call data.json(). Realistically, I don't expect this will happen anytime soon to the github API under normal circumstances, but I think it would be a good general best practice to avoid making the PEP pages dependent on an external API returning a parse-able JSON response every time.
In general, I think it would also be better to catch requests.exceptions.RequestException instead of Exception, but I don't think it's an overly substantial concern in this case.
That would be nice, but current solution looks good to me as well. Perhaps we could pass I'll try to get this merge this weekend. |
|
The PEPs now have their own repo: https://github.com/python/peps and own website: https://peps.python.org/ and also have "Last modified" footers that link to GitHub. For example: https://peps.python.org/pep-0750/ @Mariatta Please could you close this PR? |
Obtain the last modified date by making GET request to GitHub.
Don't render anything if there's any error with the GET request.
Add tests, and mock the network call.
API call: https://api.github.com/repos/python/peps/commits?path=pep-0012.txt
Display the date from
["commit"]["committer"]["date"]The url is the GitHub history of the pep file, e.g. https://github.com/python/peps/commits/master/pep-0012.txt
Closes #1564