Issue 5148261828526080: Issue 2119 - Add get_page_content template function. (Closed)
|
Created:
April 5, 2015, 12:03 p.m. by kzar Modified:
April 14, 2015, 5:16 p.m. CC:
saroyanm Visibility:
Public. |
DescriptionIssue 2119 - Add get_page_content template function.
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed Sebastian's feedback. #
Total comments: 4
Patch Set 3 : Addressed Wladimir's feedback. #Patch Set 4 : get_page_content needs to return page title as well. #
Total comments: 4
Patch Set 5 : Addressed further comments. #
Total comments: 2
Patch Set 6 : Use for, break, else control flow for page format guessing. #
Total comments: 2
Patch Set 7 : Use .iterkeys() instead of .keys() #MessagesTotal messages: 15
Patch Set 1 http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:347: from utils import get_page_params (I import this here as we have a circular dependency between utils.py and converters.py.) http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:350: params = get_page_params(self._params["source"], locale, page, format) (I'm unsure if I should pass self._params["site_url"] through here as the site_url_override keyword argument to get_page_params or not. I couldn't find where the site_url was even being used.) http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/utils.py:20: def get_page_params(source, locale, page, format, site_url_override=None): Couldn't think of a great name for this function, I'm open to suggestions.
http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:347: from utils import get_page_params On 2015/04/05 12:10:12, kzar wrote: > (I import this here as we have a circular dependency between utils.py and > converters.py.) This is usually an indicator, that something is wrong wit the structure of your code. How about moving get_page_params into this (or a new) module? http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:349: page, format = page.rsplit(".") I suppose you forgot the second argument to .rsplit()? However, I would use os.path.splitext() instead anyway. http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:350: params = get_page_params(self._params["source"], locale, page, format) Shouldn't you fallback to self._params["locale"] if locale is not given? http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:351: return params["head"], params["body"] I would rather return a dict here, to make the calling code more obvious: return {k: params[k] for k in ("head", "body")}
Patch Set 2 : Addressed Sebastian's feedback. http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:347: from utils import get_page_params On 2015/04/05 12:50:17, Sebastian Noack wrote: > On 2015/04/05 12:10:12, kzar wrote: > > (I import this here as we have a circular dependency between utils.py and > > converters.py.) > > This is usually an indicator, that something is wrong wit the structure of your > code. How about moving get_page_params into this (or a new) module? I thought about that, and maybe I'm wrong, but I thought in this situation the structure is OK. It's more a reflection of the recursive functionality. (Processing one page requires the processing of another page.) http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:349: page, format = page.rsplit(".") On 2015/04/05 12:50:17, Sebastian Noack wrote: > I suppose you forgot the second argument to .rsplit()? However, I would use > os.path.splitext() instead anyway. Done. http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:350: params = get_page_params(self._params["source"], locale, page, format) On 2015/04/05 12:50:17, Sebastian Noack wrote: > Shouldn't you fallback to self._params["locale"] if locale is not given? Done. http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:351: return params["head"], params["body"] On 2015/04/05 12:50:17, Sebastian Noack wrote: > I would rather return a dict here, to make the calling code more obvious: > > return {k: params[k] for k in ("head", "body")} Done.
LGTM http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5629499534213120/cms/... cms/converters.py:347: from utils import get_page_params On 2015/04/05 13:25:08, kzar wrote: > On 2015/04/05 12:50:17, Sebastian Noack wrote: > > On 2015/04/05 12:10:12, kzar wrote: > > > (I import this here as we have a circular dependency between utils.py and > > > converters.py.) > > > > This is usually an indicator, that something is wrong wit the structure of > your > > code. How about moving get_page_params into this (or a new) module? > > I thought about that, and maybe I'm wrong, but I thought in this situation the > structure is OK. It's more a reflection of the recursive functionality. > (Processing one page requires the processing of another page.) Cyclic dependencies aren't great. However, the logic here doesn't require cyclic dependencies it merely comes from how the code is structured. But it's not important.
http://codereview.adblockplus.org/5148261828526080/diff/5724160613416960/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5724160613416960/cms/... cms/converters.py:347: from utils import get_page_params That's an implicit relative import. From https://www.python.org/dev/peps/pep-0008/: > Implicit relative imports should never be used and have been removed in Python 3. So please: from cms.utils import get_page_params http://codereview.adblockplus.org/5148261828526080/diff/5724160613416960/cms/... cms/converters.py:349: page, ext = os.path.splitext(page) We never specify the format as part of the page. You should "guess" the format: source = self._params["source] for format in converters.iterkeys(): if source.has_page(source, format): ... In fact, that's something that get_page_params() could do.
Patch Set 3 : Addressed Wladimir's feedback. http://codereview.adblockplus.org/5148261828526080/diff/5724160613416960/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5724160613416960/cms/... cms/converters.py:347: from utils import get_page_params On 2015/04/06 19:14:30, Wladimir Palant wrote: > That's an implicit relative import. From > https://www.python.org/dev/peps/pep-0008/: > > > Implicit relative imports should never be used and have been removed in > Python 3. > > So please: > > from cms.utils import get_page_params Done. http://codereview.adblockplus.org/5148261828526080/diff/5724160613416960/cms/... cms/converters.py:349: page, ext = os.path.splitext(page) On 2015/04/06 19:14:30, Wladimir Palant wrote: > We never specify the format as part of the page. You should "guess" the format: > > source = self._params["source] > for format in converters.iterkeys(): > if source.has_page(source, format): > ... > > In fact, that's something that get_page_params() could do. Done.
Patch Set 4 : get_page_content needs to return page title as well.
http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... cms/converters.py:352: return {k: params[k] for k in ("head", "body", "title")} The CMS doesn't know about the title variable - it is handled in default.tmpl of the web.adblockplus.org repository, could be called foobar just as well. How about simply returning params unfiltered here? http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... cms/utils.py:23: format = "html" Why default to HTML? That's a legacy format, if we need a default then it should be Markdown.
Patch Set 5 : Addressed further comments. http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... cms/converters.py:352: return {k: params[k] for k in ("head", "body", "title")} On 2015/04/07 21:17:11, Wladimir Palant wrote: > The CMS doesn't know about the title variable - it is handled in default.tmpl of > the http://web.adblockplus.org repository, could be called foobar just as well. How > about simply returning params unfiltered here? Done. http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5766466041282560/cms/... cms/utils.py:23: format = "html" On 2015/04/07 21:17:11, Wladimir Palant wrote: > Why default to HTML? That's a legacy format, if we need a default then it should > be Markdown. Done.
http://codereview.adblockplus.org/5148261828526080/diff/5653164804014080/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5653164804014080/cms/... cms/utils.py:26: format = fmt I'd prefer to break when we found a match. Then we only need one variable and can make it more obvious that we use "md" as default: for format in converters: if source.has_page(page, format): break else: format = "md" Also do we actually need to fallback to "md" here, or shouldn't we better raise an exception when we don't find a match?
Patch Set 6 : Use for, break, else control flow for page format guessing. http://codereview.adblockplus.org/5148261828526080/diff/5653164804014080/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5653164804014080/cms/... cms/utils.py:26: format = fmt On 2015/04/08 08:38:57, Sebastian Noack wrote: > I'd prefer to break when we found a match. Then we only need one variable and > can make it more obvious that we use "md" as default: > > for format in converters: > if source.has_page(page, format): > break > else: > format = "md" > > Also do we actually need to fallback to "md" here, or shouldn't we better raise > an exception when we don't find a match? Cool, I've never seen an `else` clause for a `for` loop before! Done. Setting the default of "md" here is fine I think, we will then throw the correct exception ("IOError: [Errno 2] No such file or directory: './pages/example.md'"). It was just without setting a default it would appear broken when complaining that "./pages/example.None" didn't exist.
http://codereview.adblockplus.org/5148261828526080/diff/5709068098338816/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5709068098338816/cms/... cms/utils.py:23: for format in converters.keys(): Nit: This creates a new list with all the keys in the dictionary, which is unneeded here. So you should use iterkeys() instead. (You could also just iterate over the dictionary itself, which lazily yields its keys as well. But I think Wladimir prefer to explicitly request the keys.)
Patch Set 7 : Use .iterkeys() instead of .keys() http://codereview.adblockplus.org/5148261828526080/diff/5709068098338816/cms/... File cms/utils.py (right): http://codereview.adblockplus.org/5148261828526080/diff/5709068098338816/cms/... cms/utils.py:23: for format in converters.keys(): On 2015/04/08 09:09:32, Sebastian Noack wrote: > Nit: This creates a new list with all the keys in the dictionary, which is > unneeded here. So you should use iterkeys() instead. > > (You could also just iterate over the dictionary itself, which lazily yields its > keys as well. But I think Wladimir prefer to explicitly request the keys.) Done.
LGTM
LGTM 
|
