Issue 1321 - Truncate extension name and description at build time instead of only truncating descr…

Issue 1321 - Truncate extension name and description at build time instead of only truncating descr…

[Sebastian Noack, 2014-09-03 18:53:37]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
I think we should at least generate a warning when truncating the text.

[Wladimir Palant, 2014-09-03 19:05:22]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/03 18:53:37, Sebastian Noack wrote:
> I think we should at least generate a warning when truncating the text.

I considered doing that. However, the main effect of this warning would be
spamming Felix and me (we are the ones receiving cron mail from development
build generation). I'd really prefer to have this covered by
https://issues.adblockplus.org/ticket/104 instead.

[Sebastian Noack, 2014-09-03 19:13:02]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/03 19:05:22, Wladimir Palant wrote:
> On 2014/09/03 18:53:37, Sebastian Noack wrote:
> > I think we should at least generate a warning when truncating the text.
> 
> I considered doing that. However, the main effect of this warning would be
> spamming Felix and me (we are the ones receiving cron mail from development
> build generation).

Perfect. Even better would be to have proper monitoring, where everybody can see
if/what went wrong. But for now that should be fine.

> I'd really prefer to have this covered by
> https://issues.adblockplus.org/ticket/104 instead.

It shouldn't take until the next release to realize that a translation were
truncated. Also this issue won't probably be done in the near future.

[Wladimir Palant, 2014-09-03 21:21:31]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/03 19:13:03, Sebastian Noack wrote:
> Perfect.

Not at all. It should spam you because you are the one who should do something
about it.

> It shouldn't take until the next release to realize that a translation were
> truncated.

A reminder: translations are being updated manually. Whoever updated them should
also check them. And not rely on Felix or me to be annoyed enough to remind him
of it.

[Sebastian Noack, 2014-09-04 10:40:02]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/03 21:21:31, Wladimir Palant wrote:
> On 2014/09/03 19:13:03, Sebastian Noack wrote:
> > Perfect.
> 
> Not at all. It should spam you because you are the one who should do something
> about it.
> 
> > It shouldn't take until the next release to realize that a translation were
> > truncated.
> 
> A reminder: translations are being updated manually. Whoever updated them
should
> also check them. And not rely on Felix or me to be annoyed enough to remind
him
> of it.

So I feel this is the wrong place to address that issue. Why don't we just make
"build.py gettranslations" ensure the length of those string?

[Wladimir Palant, 2014-09-04 13:27:03]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/04 10:40:02, Sebastian Noack wrote:
> So I feel this is the wrong place to address that issue. Why don't we just
make
> "build.py gettranslations" ensure the length of those string?

That's what you did originally. But that means that we have a truncated string
in our repository and need to go to Crowdin to see the real one (e.g. to shorten
it). Also, we will have to truncate it for all browsers, not just Chrome.

gettranslations really shouldn't change the data...

[Sebastian Noack, 2014-09-04 13:29:55]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/04 13:27:03, Wladimir Palant wrote:
> On 2014/09/04 10:40:02, Sebastian Noack wrote:
> > So I feel this is the wrong place to address that issue. Why don't we just
> make
> > "build.py gettranslations" ensure the length of those string?
> 
> That's what you did originally. But that means that we have a truncated string
> in our repository and need to go to Crowdin to see the real one (e.g. to
shorten
> it). Also, we will have to truncate it for all browsers, not just Chrome.
> 
> gettranslations really shouldn't change the data...

I agree, but that isn't what I meant to suggest. gettranslations should fail
with an error (or at least show a warning) if the string is too long. So whoever
imports the translation (prior to the release in the future) must take care of
it.

[Wladimir Palant, 2014-09-04 13:47:43]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/04 13:29:55, Sebastian Noack wrote:
> I agree, but that isn't what I meant to suggest. gettranslations should fail
> with an error (or at least show a warning) if the string is too long. So
whoever
> imports the translation (prior to the release in the future) must take care of
> it.

Take care how exactly? Do you speak Telugu? I don't. And if there is an issue in
the Telugu translation (one that cannot be addressed by removing whitespace),
then the usual approach is to inform the translator. If we are lucky, the
translator will respond on the same day. It's quite likely however that this
issue won't be fixed in time for the release (maybe not even in time for the
next one). I'd prefer to be able to release nevertheless, merely with a
truncated string.

As I said, warnings are something that should be addressed in #104 - that
validation can be integrated into gettranslations then (or maybe not, the
validation on Firefox produces a massive amount of warnings). It shouldn't be
hardcoded that "name_devbuild" can only be 45 characters long, this is something
to be configured in metadata.common. Note also that validating "name" requires
actually importing it from Firefox.

[Sebastian Noack, 2014-09-04 14:00:58]

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
File packagerChrome.py (right):

http://codereview.adblockplus.org/5709087064981504/diff/5629499534213120/pack...
packagerChrome.py:294: return text[:length_limit - 1].rstrip() + u"\u2026"
On 2014/09/04 13:47:43, Wladimir Palant wrote:
> On 2014/09/04 13:29:55, Sebastian Noack wrote:
> > I agree, but that isn't what I meant to suggest. gettranslations should fail
> > with an error (or at least show a warning) if the string is too long. So
> whoever
> > imports the translation (prior to the release in the future) must take care
of
> > it.
> 
> Take care how exactly? Do you speak Telugu? I don't. And if there is an issue
in
> the Telugu translation (one that cannot be addressed by removing whitespace),
> then the usual approach is to inform the translator. If we are lucky, the
> translator will respond on the same day. It's quite likely however that this
> issue won't be fixed in time for the release (maybe not even in time for the
> next one). I'd prefer to be able to release nevertheless, merely with a
> truncated string.
> 
> As I said, warnings are something that should be addressed in #104 - that
> validation can be integrated into gettranslations then (or maybe not, the
> validation on Firefox produces a massive amount of warnings). It shouldn't be
> hardcoded that "name_devbuild" can only be 45 characters long, this is
something
> to be configured in metadata.common. Note also that validating "name" requires
> actually importing it from Firefox.

In the worst case one can still decide to truncate the string manually. But it
dangerous, to silently truncate strings.

As I said yesterday, I think we should update the translations at least one week
before the release, that there is enough time to resolve issues like that an
similar ones.

[Wladimir Palant, 2014-09-04 14:39:52]

On 2014/09/04 14:00:58, Sebastian Noack wrote:
> In the worst case one can still decide to truncate the string manually. But it
> dangerous, to silently truncate strings.

It's more dangerous to mess with strings manually. Fact is, right now we *do*
truncate strings silently. We should definitely address this in #104, this issue
isn't the place however.

> As I said yesterday, I think we should update the translations at least one
week
> before the release, that there is enough time to resolve issues like that an
> similar ones.

We are working with volunteer translators, they don't come with availability
guarantees. It is very typical that translators spend some time on a weekend and
won't address any issues for a month. In fact, they might not even care enough
to address any issues at all, ever.

Note that you didn't address any of my technical concerns either:
gettranslations isn't the right place to implement this, at least not without
significant architectural changes.

[Sebastian Noack, 2014-09-04 16:24:44]

LGTM, I guess.