impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <>
Subject [Impala-ASF-CR](asf-site) Initial commit of the blog section of the Impala ASF website.
Date Sun, 06 Nov 2016 21:53:48 GMT
David Knupp has posted comments on this change.

Change subject: Initial commit of the blog section of the Impala ASF website.

Patch Set 1:

Commit Message:

PS1, Line 16: work properly
> "work properly if Pelican is to be used for non-blog pages -- a job"

PS1, Line 25: comments section
> This is presumably not possible with a static site generator without disqus
Exactly. Disqus seems to be a popular approach.
File Makefile:

Line 1: PY?=python
> This deserves comments.
To get this up quickly for people to start adding articles, my approach was to just do the
bare bones, default creation of a Pelican site, and not invest too much time in tweaking or
pruning it. I didn't want to hastily delete something, and if/when we decide to spend more
time on the site later, have to then re-spawn or re-fetch a file that had been removed earlier.

By default, you get this Makefile and the file when you create a new site.
We actually don't use ANY of it, so I haven't touched a single line. I'll go ahead and delete
File blog/

Line 5: 
> extra blank line?

PS1, Line 20:  path/to/venv
> What is this path? How do I find it?
Ah. It can be any path you want it to be. This is the canonical way that a typical python
developer works with virtual environments. You use the virtualenv tool to create a new virtual
environment some place that makes sense to you, which you can then activate and deactivate
as needed. It's common to have multiple isolated environments for different projects, that
may each have different versions of common libs, or even different versions of the python

E.g, if you run:

   $ source $IMPALA_HOME/infra/python/env/bin/activate
   (env) $

then anywhere you are, whenever you invoke plain ol' python, you're actually using the executable
inside infra/python/env/bin/ that we're accustomed to invoking indirectly via the 'impala-python'

   (env) $ which python

I've updated the README to provide more information about using virtualenv.

PS1, Line 24: (impala_blog)
> What is this?
See updated README.

Line 32: Articles are stored as markdown files in the ```blog_content/``` directory, and follow
a specific template. The name of the file is not important, as long as it's unique. You sould
probably create this file on a new working branch in the repo.
> Long line, here and below

Line 108: Don't forget to ```deactivate``` your virtualenv when you're done working on the
> How?
See updated README.

Line 1: #!/usr/bin/env python
> licence header, please
I wasn't sure what to do about this. I noticed that while some of our third party code includes
a license header:

Some of our third party code doesn't:

Can you tell me explicitly what to put here? (FYI, Pelican has the AGPL license:

Line 3: from __future__ import unicode_literals
> Why?
This is code that many developers put at the beginning of their python files to ease transition
from 2 to 3. Python 3 has several backward-breaking changes. The standard list of __future__
imports actually includes more. There's also absolute_import, division, and print_function.
(Aside: if we ever have to support Python 3 in Impala, we may be somewhat screwed. For starters,
I see a lot print statements liberally scattered throughout our code base that will all raise
exceptions under python 3.x.)

I didn't actually put this line in this file. When the file was auto-spawned, it was there.
All I did in this file was provide meaningful values for a few of the settings. For more specific
info on this line, see for more

Line 5: AUTHOR = u'Impala Dev'
> Does this need to be customized?
More than it is?  The docs said to make this setting the "default author." Blog posts will
still respect the Author: field in markdown files. I think it only gets used of we don't specify
a blog author, or if we explicitly use {{AUTHOR}} on one of the template files.

Line 22: # Blogroll
> These could be removed or more clearly explained
Boilerplate that came when this file was spawned. (A lot of blogs have a blogroll.) Removed.

> 100 posts per page?
Oops. I think that was supposed to be 10. How about 10?

Line 38: # Uncomment following line if you want document-relative URLs when developing
> Why not?
Links might get screwy with half the site being pelican managed, and the other half not, especially
considering the fact that pages might be nested at different levels. For example, with the
static nav bar in the main page template, what happens when some pages are at the root level,
some might be in the blog/ directory, and still others might (at some point) be in the blog/categories/
directory? Absolute URLs worked out-of-the-box.

Also, the docs said this, so I left it at the default (which was commented out.)

"Defines whether Pelican should use document-relative URLs or not. Only set this to True when
developing/testing and only if you fully understand the effect it can have on links/feeds."
File themes/impala_asf_site_theme/templates/base.html:

Line 2: <html lang="{{ DEFAULT_LANG }}">
> It would be good if this file mentioned in an HTML comment at the top that 
Actually, this being a template file, it's exactly a file that gets hand-edited. It's from
files like this (and the others in the template directory) that the actual site files -- index.html,
my-blog-post.html, etc. -- are generated. In other words, let's say some day the Github address
changes -- from "incubator-impala" to just "impala." This is the file where we'd edit the
link in the nav bar. Then we'd run pelican, and the change would propagate to all of the individual
page files that pelican manages.
File themes/impala_asf_site_theme/templates/gosquared.html:

> What is this all about?
We needed template files, and there's a "starter" theme that you can get with Pelican that
you can modify.

I only modified the templates we obviously needed, and left the others untouched -- assuming
they might be useful later. (e.g., once we get enough articles, the pagination template will
probably come into play.) Admittedly, I didn't look at each template file too closely, or
test every contingency. However, this one in particular seems like it won't ever be used.
I'll delete it.
File themes/impala_asf_site_theme/templates/page.html:

Line 12: 		Last updated: {{ page.locale_modified }}
> We generally avoid tabs
Ah, good eye. Again, this is a standard file that came with the theme, and I didn't actually
edit this one.
File themes/impala_asf_site_theme/templates/tag.html:

> What is this for?
Weird, right?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa578a70237fcc97589c667c17a70d3d6dad5ae1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: David Knupp <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-HasComments: Yes

View raw message