Code review comment for lp:~diegosarmentero/ubuntuone-control-panel/checkwrap

Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

> Two comments,
>
> 1. I have been told (I' not sure how official is this) that we should start
> making some move towards using python 3, due to that we should stop using the
> old string formating 'Hola %s' % 'gatox' to the new one 'Hola
> {0}'.format('gatox'). Can you please do that.
>

Stay as it is because we didn't decide how to use yet.

> 2. I'm not sure about the removal of the \n in the text.. In translations they
> do not care about the length of the string but they do care about the
> paragraph separations. What I mean is:
>
> 'This is a very long text, lets say I use more than one paragraph to separater
> ideas.\n\nThis is my new paragraph focusing on a diff idea.'
>
> In these type of cases we are breaking the paragraph separation that is used
> by the translator with the intent to express to diff ideas. What I would do is
> not to remove \n and to check at in every word if we have a \n at the end
> (word[-1] is faster than endswith.) if that is the case we should set the size
> to be the full line size.
>

Fixed, the '\n' is not needed because we are always providing the original string for the wrapping method.

> 3. Since this is a very generic method (can be used in any possible widget)
> and there is no limit in the length of the strings to be wrapped I would
> assume the worst case scenario which is the case in which strings are very
> long. If that is the case using += to create the final text is probably not
> the best thing to do. Following the recommendations from
> http://wiki.python.org/moin/PythonSpeed/PerformanceTips#String_Concatenation I
> would at least move to .join instead of +=.

Changed.

« Back to merge proposal