Skip to content

Commit

Permalink
Do only 1 github API request when requesting rendered html for guide
Browse files Browse the repository at this point in the history
- The github API doesn't allow us to fetch the file's SHA and rendered HTML in
  a single request.  Previously we were hiding this in the remote layer and
  doing 2 requests for every call that requested the rendered text.  This is
  fairly wasteful since we don't ever use the SHA for rendered_text calls
  anyway.
- Now the lower-level remote.py API does not secretly do both requests but
  callers are able to make both if they choose so the number of API requests
  from the outside world is more explicit.
  • Loading branch information
durden committed Apr 11, 2016
1 parent ff692ed commit 8793949
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 7 deletions.
6 changes: 5 additions & 1 deletion pskb_website/models/article.py
Expand Up @@ -299,7 +299,11 @@ def read_article(path, rendered_text=True, branch=u'master', repo_path=None,

details = remote.read_file_from_github(full_path, branch, rendered_text,
allow_404=allow_missing)
if details is None or None in (details.text, details.sha):

# Allow empty sha when requesting rendered_text b/c of the way the
# underlying remote API works. See read_file_from_github for more
# information.
if details is None or details.text is None or (details.sha is None and not rendered_text):
if not allow_missing:
app.logger.error('Failed reading path: "%s" branch: %s', full_path,
branch)
Expand Down
25 changes: 19 additions & 6 deletions pskb_website/remote.py
Expand Up @@ -251,16 +251,29 @@ def read_file_from_github(path, branch=u'master', rendered_text=True,
:param allow_404: False to log warning for 404 or True to allow it i.e.
when you're just seeing if a file already exists
:returns: file_details namedtuple or None if error
"""
details = file_details_from_github(path, branch, allow_404=allow_404)
if details is None:
return details
Note when requesting rendered text there will be no SHA or last_updated
data available. This is a restriction from the github API
(https://developer.github.com/v3/media/#repository-contents) Requesting
file 'details' like SHA and rendered text are 2 API calls. Therefore, if
you want all of that information you should call this function twice, once
with rendered_text=True and one with rendered_text=False and combine the
information yourself.
"""

if rendered_text:
text = rendered_markdown_from_github(path, branch)
details = file_details(path, branch, details.sha, details.last_updated,
details.url, text)

# This is a little tricky b/c this URL could change on github and we
# would be wrong. However, those URLs have been the same for years so
# seems like a safe enough bet at this point.
owner, repo, file_path = split_full_file_path(path)
url = u'https://github.com/%s/%s/blob/%s/%s' % (owner, repo, branch,
file_path)

details = file_details(path, branch, None, None, url, text)
else:
details = file_details_from_github(path, branch, allow_404=allow_404)

return details

Expand Down

0 comments on commit 8793949

Please sign in to comment.