Issue 2340 - Don`t generate pages if less than 30% have been translated

Issue 2340 - Don`t generate pages if less than 30% have been translated

[Wladimir Palant, 2015-04-17 17:22:31]

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/utils.py:63: for l in source.list_locales()
Parameter being overwritten here isn't quite related, this issue made me waste
time when testing however.

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/utils.py:76: params["translation_ratio"] = 1
Note that having this in params means that default.tmpl can access it - so we
could show a message for incomplete translations along the lines of "Help us
finish this translation on Crowdin."

[Sebastian Noack, 2015-04-17 17:32:27]

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
File cms/bin/generate_static_pages.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/bin/generate_static_pages.py:22: MIN_TRANSLATED = 0.3
Is this actually the same threshold we currently use with Anwiki?

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/utils.py:72: if converter.total_translations:
Nit: total_translation > 0

I know 0 evaluates to False. But semantically we aren't checking for presence
here, but the logic actually is about this being larger than 0.

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/utils.py:84: else:
Nit: Redundant else statement.

[Wladimir Palant, 2015-04-17 18:21:15]

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
File cms/bin/generate_static_pages.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/bin/generate_static_pages.py:22: MIN_TRANSLATED = 0.3
On 2015/04/17 17:32:27, Sebastian Noack wrote:
> Is this actually the same threshold we currently use with Anwiki?

Yes. The effect might still be different however, the strings are broken up
differently.

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...
cms/utils.py:84: else:
On 2015/04/17 17:32:27, Sebastian Noack wrote:
> Nit: Redundant else statement.

I thought you would object to this, but I actually like redundant else
statements :)

[Sebastian Noack, 2015-04-18 08:04:34]

LGTM

[Wladimir Palant, 2015-04-18 08:56:50]

On 2015/04/18 08:04:34, Sebastian Noack wrote:
> LGTM

Not quite unfortunately. I realized that this will produce dead links - pages
with less than 30% translations shouldn't be linked to. I'll create a new patch
later.

[Wladimir Palant, 2015-05-06 15:20:52]

I've changed the approach here and rebased, links to non-existing pages
shouldn't be generated any more. Not quite happy with the code duplication
however.

[Sebastian Noack, 2015-05-06 16:15:30]

I'm not quite sure whether it's a great idea to have inconsistent behavior when
generating links in the development server and when generating the pages. Why
don't you move that logiy into Source.has_locale, rather than lazy patching it
during page generation? Beside consistent behavior, it will also eliminate quite
a few hacks.

On 2015/05/06 15:20:52, Wladimir Palant wrote:
> Not quite happy with the code duplication however.
Which code duplication are you talking about?

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/bin/generate_static_pages.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:39: wrapper.original = func
I'm not sure if I like that hack. Note that it wouldn't be neccessarry if you
would pass the dict object used for caching as an argument:

  cached_links = {}
  source.resolve_link = memoize(source.resolve_link, cached_links)

  ...

  cached_links.clear()

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:101: else:
Nit: Redundant else statement. (Yes, I know you like them)

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:103: source.has_locale =
functools.partial(has_locale, source.has_locale)
It took me a while to understand what you are doing here. Would be more clear
IMO, if you just use a closure variable:

  orig_has_locale = source.has_locale
  def has_locale(locale, page):
    ..
    return orig_has_locale(locale, page)
  source.has_locale = has_locale

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/sources.py (left):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/sources.py:42: page = config.get("locale_overrides", page)
So locale overrides are now only repsected when generating the pages but no
longer when running the development server? :(

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/utils.py:62: l
Why didn't you keep the more descriptive variable name, as you wrap the lines
anyway? Not that this change seems to be related anyway.

[Wladimir Palant, 2015-05-06 17:21:59]

On 2015/05/06 16:15:30, Sebastian Noack wrote:
> I'm not quite sure whether it's a great idea to have inconsistent behavior
when
> generating links in the development server and when generating the pages. Why
> don't you move that logiy into Source.has_locale, rather than lazy patching it
> during page generation? Beside consistent behavior, it will also eliminate
quite
> a few hacks.

The development server should by definition show all pages, even the ones with
only one translation - otherwise there would be no way to test them.

Besides, how would we move the logic into Source.has_locale()? The only way to
know whether the locale is complete enough is rendering the page - and not
everything getting there is even an actual page. Having this helper process
render the page is quite a hack, and the performance is rather horrible even
with caching.

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/bin/generate_static_pages.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:39: wrapper.original = func
On 2015/05/06 16:15:30, Sebastian Noack wrote:
> I'm not sure if I like that hack.

True, I don't like passing in a dictionary either however. Given that it can
only really be used to clear the cache, I can just expose that functionality.

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:101: else:
On 2015/05/06 16:15:30, Sebastian Noack wrote:
> Nit: Redundant else statement. (Yes, I know you like them)

Done and made the logic here slightly more obvious.

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:103: source.has_locale =
functools.partial(has_locale, source.has_locale)
On 2015/05/06 16:15:30, Sebastian Noack wrote:
> It took me a while to understand what you are doing here. Would be more clear
> IMO, if you just use a closure variable:

I thought you were a fan of clever solutions :)

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/sources.py (left):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/sources.py:42: page = config.get("locale_overrides", page)
On 2015/05/06 16:15:30, Sebastian Noack wrote:
> So locale overrides are now only repsected when generating the pages but no
> longer when running the development server? :(

No, this logic is actually built into the default has_locale function. The block
here was unnecessarily duplicating it - I removed it to make sure it doesn't
falsify testing.

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/utils.py:62: l
On 2015/05/06 16:15:30, Sebastian Noack wrote:
> Why didn't you keep the more descriptive variable name, as you wrap the lines
> anyway? Not that this change seems to be related anyway.

Please see
http://codereview.adblockplus.org/5242593268989952/diff/5629499534213120/cms/...

[Sebastian Noack, 2015-05-06 19:12:15]

On 2015/05/06 17:21:59, Wladimir Palant wrote:
> The development server should by definition show all pages, even the ones with
> only one translation - otherwise there would be no way to test them.
> 
> Besides, how would we move the logic into Source.has_locale()? The only way to
> know whether the locale is complete enough is rendering the page - and not
> everything getting there is even an actual page. Having this helper process
> render the page is quite a hack, and the performance is rather horrible even
> with caching.

I see. I suppose it's fine then.

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/bin/generate_static_pages.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:39: wrapper.original = func
On 2015/05/06 17:21:59, Wladimir Palant wrote:
> On 2015/05/06 16:15:30, Sebastian Noack wrote:
> > I'm not sure if I like that hack.
> 
> True, I don't like passing in a dictionary either however. Given that it can
> only really be used to clear the cache, I can just expose that functionality.

Well, allowing to pass in a dict-like object, also helps other use cases, like
implementing different kind of caches (like and LRU cache or using an external
cache backend). Or to share a cache for different functions. It's actually quite
common practice to take a cache argument when implementing a memoize() function,
django for example does that.

It also has the advantage that you might not mistakenly call .clear_cache() on a
function that wasn't actually decorated with memoize(). In fact it took my a
while to figure out where the "original" argument came from.

Also note that it's common practice, at least for decorators that are supposed
to work with any callable object, to use functools.update_wrapper() to copy
attribute from the decorated function to wrapper. That's not necessary here, but
if this decorator becomes more widely used, we might want to do that. And adding
additional attributes can cause conflicts then.

However, I'm not feeling too strong about it. So LGTM if you disagree.

[Wladimir Palant, 2015-05-06 19:20:35]

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
File cms/bin/generate_static_pages.py (right):

http://codereview.adblockplus.org/5242593268989952/diff/5733935958982656/cms/...
cms/bin/generate_static_pages.py:39: wrapper.original = func
On 2015/05/06 19:12:15, Sebastian Noack wrote:
> However, I'm not feeling too strong about it. So LGTM if you disagree.

Thank you. Indeed, I do feel that the current approach is better than just
manipulating the dictionary used by memoize() and potentially breaking
assumptions that this decorator makes.