Issue 29347181: Issue 4217 - Fix Prefix Regression Due To Mercurial Version Behavior Change (Closed)
|
Created:
July 1, 2016, 11:04 a.m. by Jon Sonesen Modified:
July 1, 2016, 3:20 p.m. Visibility:
Public. |
Descriptionfixes mercurial version regression on prefix, and adds format method to strings
Patch Set 1 #
Total comments: 1
Patch Set 2 : remove legacy code replacement #
Total comments: 5
Patch Set 3 : added + operator concat addressing previous comments #Patch Set 4 : adds format operator to string forat #MessagesTotal messages: 18
https://codereview.adblockplus.org/29347181/diff/29347182/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29347181/diff/29347182/cms/sources.py#newc... cms/sources.py:61: path = '/{}/{}'.format(locale, page) Replacing legacy string formatting (using the % operator) with the format() method across the file is an unrelated change and should rather go into a separate review and commit.
On 2016/07/01 11:18:47, Sebastian Noack wrote: > https://codereview.adblockplus.org/29347181/diff/29347182/cms/sources.py > File cms/sources.py (right): > > https://codereview.adblockplus.org/29347181/diff/29347182/cms/sources.py#newc... > cms/sources.py:61: path = '/{}/{}'.format(locale, page) > Replacing legacy string formatting (using the % operator) with the format() > method across the file is an unrelated change and should rather go into a > separate review and commit. You are right, I will fix it :D
remove legacy code replacement
remove legacy code replacement
https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... cms/sources.py:230: self._archive.getinfo('root/%s' % filename) In the case here, it would be preferable to simply use the + operator: 'root/' + filename From our coding style guide: Use the + operator when concatenating exactly two strings, use the format() method for more complex string formatting https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... cms/sources.py:236: data = self._archive.read('root/%s' % filename) Same here. https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... cms/sources.py:242: prefix = 'root/%s/' % subdir Please use format() rather than % in new code.
On 2016/07/01 11:41:12, Sebastian Noack wrote: > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py > File cms/sources.py (right): > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > cms/sources.py:230: self._archive.getinfo('root/%s' % filename) > In the case here, it would be preferable to simply use the + operator: > > 'root/' + filename > > From our coding style guide: > > Use the + operator when concatenating exactly two strings, > use the format() method for more complex string formatting > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > cms/sources.py:236: data = self._archive.read('root/%s' % filename) > Same here. > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > cms/sources.py:242: prefix = 'root/%s/' % subdir > Please use format() rather than % in new code. ok, I can change this but I did not because similarly to the previous style changes it seemed unrelated but since it is not file wide I guess that is acceptable?
https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... cms/sources.py:242: prefix = 'root/%s/' % subdir On 2016/07/01 11:43:08, Jon Sonesen wrote: > On 2016/07/01 11:41:12, Sebastian Noack wrote: > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py > > File cms/sources.py (right): > > > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > > cms/sources.py:230: self._archive.getinfo('root/%s' % filename) > > In the case here, it would be preferable to simply use the + operator: > > > > 'root/' + filename > > > > From our coding style guide: > > > > Use the + operator when concatenating exactly two strings, > > use the format() method for more complex string formatting > > > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > > cms/sources.py:236: data = self._archive.read('root/%s' % filename) > > Same here. > > > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > > cms/sources.py:242: prefix = 'root/%s/' % subdir > > Please use format() rather than % in new code. > > ok, I can change this but I did not because similarly to the previous style > changes it seemed unrelated but since it is not file wide I guess that is > acceptable? It's not unrelated here, bacuase you are changing/replacing this code anyway. And new code should always follow our latest coding practices.
On 2016/07/01 11:46:46, Sebastian Noack wrote: > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py > File cms/sources.py (right): > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > cms/sources.py:242: prefix = 'root/%s/' % subdir > On 2016/07/01 11:43:08, Jon Sonesen wrote: > > On 2016/07/01 11:41:12, Sebastian Noack wrote: > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py > > > File cms/sources.py (right): > > > > > > > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > > > cms/sources.py:230: self._archive.getinfo('root/%s' % filename) > > > In the case here, it would be preferable to simply use the + operator: > > > > > > 'root/' + filename > > > > > > From our coding style guide: > > > > > > Use the + operator when concatenating exactly two strings, > > > use the format() method for more complex string formatting > > > > > > > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > > > cms/sources.py:236: data = self._archive.read('root/%s' % filename) > > > Same here. > > > > > > > > > https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... > > > cms/sources.py:242: prefix = 'root/%s/' % subdir > > > Please use format() rather than % in new code. > > > > ok, I can change this but I did not because similarly to the previous style > > changes it seemed unrelated but since it is not file wide I guess that is > > acceptable? > > It's not unrelated here, bacuase you are changing/replacing this code anyway. > And new code should always follow our latest coding practices. Thanks for clearing that up I will remember this moving forward
added + operator concat addressing previous comments
On 2016/07/01 11:57:29, Jon Sonesen wrote: > added + operator concat addressing previous comments Dang, sorry I didn't add the format operator! sorry
https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29347181/diff/29347186/cms/sources.py#newc... cms/sources.py:242: prefix = 'root/%s/' % subdir On 2016/07/01 11:41:11, Sebastian Noack wrote: > Please use format() rather than % in new code. What is about this comment?
LGTM
Message was sent while issue was closed.
Jon, Technically if I'm in the reviewers, you need my LGTM too. But in this case, no problem, here it is: LGTM :)
Message was sent while issue was closed.
Well, I pushed it.
Message was sent while issue was closed.
On 2016/07/01 15:12:22, Vasily Kuznetsov wrote: > Jon, > > Technically if I'm in the reviewers, you need my LGTM too. But in this case, no > problem, here it is: > > LGTM > > :) Thank you for clarifying!
Message was sent while issue was closed.
On 2016/07/01 15:12:57, Sebastian Noack wrote: > Well, I pushed it. Yeah, I saw it already. No worries in this case, it's a simple change and having two reviewers probably was an overkill anyway. 
|
