Issue 5331 - Adds has_string() global

Looks like this may be the "dumb" solution but perhaps it is not so bad considering it requires no additional changes to the code base, lowering a chance of regression. Open to changing though

[Vasily Kuznetsov, 2017-06-20 17:38:28]

Pretty cool! It turned out to be simpler than I thought.

Just have a few small suggestions below.

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
cms/converters.py:441: default = localedata[name]
I didn't pay attention to this line when looking into this. Actually you seem to
be right and checking that the string exists is much simpler than I thought.

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
cms/converters.py:450: localedata =
self._params['source'].read_locale(self._params['locale'],
Maybe create a small protected method for this logic to not replicate it? For
example `_get_localedata(self, page)`

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
cms/converters.py:452: if name in localedata:
`return name in localedata` would do the same in one line.

https://codereview.adblockplus.org/29469568/diff/29469569/tests/test_site/pag...
File tests/test_site/pages/global.tmpl (right):

https://codereview.adblockplus.org/29469568/diff/29469569/tests/test_site/pag...
tests/test_site/pages/global.tmpl:3: {{- has_string('temp') }}
Perhaps add another one for a string that exists?

[Jon Sonesen, 2017-06-21 07:53:54]

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
cms/converters.py:450: localedata =
self._params['source'].read_locale(self._params['locale'],
On 2017/06/20 17:38:27, Vasily Kuznetsov wrote:
> Maybe create a small protected method for this logic to not replicate it? For
> example `_get_localedata(self, page)`

yeah, this is good. but what about the if page is none logic? Basically, it
seems it would be redundant to put in the new method unless we remove it here.
the problem there is that we still have casees where the page will be none,
however the protected method could always return the page and the local data.
What do you think?

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
cms/converters.py:452: if name in localedata:
On 2017/06/20 17:38:28, Vasily Kuznetsov wrote:
> `return name in localedata` would do the same in one line.

Acknowledged.

https://codereview.adblockplus.org/29469568/diff/29469569/tests/test_site/pag...
File tests/test_site/pages/global.tmpl (right):

https://codereview.adblockplus.org/29469568/diff/29469569/tests/test_site/pag...
tests/test_site/pages/global.tmpl:3: {{- has_string('temp') }}
On 2017/06/20 17:38:28, Vasily Kuznetsov wrote:
> Perhaps add another one for a string that exists?

Acknowledged.

[Vasily Kuznetsov, 2017-06-21 08:17:00]

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
cms/converters.py:450: localedata =
self._params['source'].read_locale(self._params['locale'],
On 2017/06/21 07:53:54, Jon Sonesen wrote:
> On 2017/06/20 17:38:27, Vasily Kuznetsov wrote:
> > Maybe create a small protected method for this logic to not replicate it?
For
> > example `_get_localedata(self, page)`
> 
> yeah, this is good. but what about the if page is none logic? Basically, it
> seems it would be redundant to put in the new method unless we remove it here.
> the problem there is that we still have casees where the page will be none,
> however the protected method could always return the page and the local data.
> What do you think?

Yeah, all true. I was thinking to leave that part in this method (and have a bit
of duplication -- this one you can't really avoid I guess). So the new method
would just have one call to blabla.read_locale(...) in it, but there's quite
some stuff in it, so it makes sense I think.

[Jon Sonesen, 2017-06-21 09:38:48]

On 2017/06/21 08:17:00, Vasily Kuznetsov wrote:
> https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py
> File cms/converters.py (right):
> 
>
https://codereview.adblockplus.org/29469568/diff/29469569/cms/converters.py#n...
> cms/converters.py:450: localedata =
> self._params['source'].read_locale(self._params['locale'],
> On 2017/06/21 07:53:54, Jon Sonesen wrote:
> > On 2017/06/20 17:38:27, Vasily Kuznetsov wrote:
> > > Maybe create a small protected method for this logic to not replicate it?
> For
> > > example `_get_localedata(self, page)`
> > 
> > yeah, this is good. but what about the if page is none logic? Basically, it
> > seems it would be redundant to put in the new method unless we remove it
here.
> > the problem there is that we still have casees where the page will be none,
> > however the protected method could always return the page and the local
data.
> > What do you think?
> 
> Yeah, all true. I was thinking to leave that part in this method (and have a
bit
> of duplication -- this one you can't really avoid I guess). So the new method
> would just have one call to blabla.read_locale(...) in it, but there's quite
> some stuff in it, so it makes sense I think.

Yeah, sounds good

[Vasily Kuznetsov, 2017-06-21 10:29:04]

LGTM