Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
GH-1564: Add "Last Modified" trailer to PEP HTML pages #1565
Conversation
Verified
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'] |
aeros
Aug 11, 2020
Member
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 |
aeros
Aug 11, 2020
Member
| 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. |
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