Issue 29549604: Issue 5729 - remove_redundant_translations fails due to the presence of system file (Closed)
|
Created:
Sept. 19, 2017, 9:58 a.m. by jens Modified:
Oct. 4, 2017, 12:43 p.m. CC:
Felix Dahlke, René Jeschke Visibility:
Public. |
DescriptionIssue 5729 - remove_redundant_translations fails due to the presence of system file
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Removed Windows part #
Total comments: 2
Patch Set 3 : removed simicolons #MessagesTotal messages: 9
On 2017/09/19 10:08:51, jens wrote: LGTM. I made just a small observation in one of the comments
https://codereview.adblockplus.org/29549604/diff/29549714/translations.py File translations.py (right): https://codereview.adblockplus.org/29549604/diff/29549714/translations.py#new... translations.py:86: # ignore system files I think you can change the comment to just 'ignore files' since it will ignore any file, no matter if it's a system file or a normal one.
Hi Jens, The changes look good in general, but I'm not quite sure about the need for the Windows part. How much does this script get run on Windows and was there ever any issue with system or hidden files there? We seem to be going into quite a bit of trouble, introducing a 3rd party dependency and all that, seemingly for a problem that might not even exist (at least it's not mentioned in the ticket). If we do need Windows support after all, have you looked into using `os.stat` (see https://docs.python.org/2/library/os.html#os.stat). It seems like it might actually give you the information that you need. Cheers, Vasily https://codereview.adblockplus.org/29549604/diff/29549714/translations.py File translations.py (right): https://codereview.adblockplus.org/29549604/diff/29549714/translations.py#new... translations.py:99: return attribute & (win32con.FILE_ATTRIBUTE_HIDDEN | win32con.FILE_ATTRIBUTE_SYSTEM) Our Python style guide limits lines to 79 characters (following PEP8, which is more or less of a universal standard). You can reformat this line like this, to make it compliant: return attribute & (win32con.FILE_ATTRIBUTE_HIDDEN | win32con.FILE_ATTRIBUTE_SYSTEM)
On 2017/09/19 21:48:05, Vasily Kuznetsov wrote: > Hi Jens, > > The changes look good in general, but I'm not quite sure about the need for the > Windows part. How much does this script get run on Windows and was there ever > any issue with system or hidden files there? We seem to be going into quite a > bit of trouble, introducing a 3rd party dependency and all that, seemingly for a > problem that might not even exist (at least it's not mentioned in the ticket). > > If we do need Windows support after all, have you looked into using `os.stat` > (see https://docs.python.org/2/library/os.html#os.stat). It seems like it might > actually give you the information that you need. > > Cheers, > Vasily > > https://codereview.adblockplus.org/29549604/diff/29549714/translations.py > File translations.py (right): > > https://codereview.adblockplus.org/29549604/diff/29549714/translations.py#new... > translations.py:99: return attribute & (win32con.FILE_ATTRIBUTE_HIDDEN | > win32con.FILE_ATTRIBUTE_SYSTEM) > Our Python style guide limits lines to 79 characters (following PEP8, which is > more or less of a universal standard). You can reformat this line like this, to > make it compliant: > > return attribute & (win32con.FILE_ATTRIBUTE_HIDDEN | > win32con.FILE_ATTRIBUTE_SYSTEM) Thanks for your feedback, Vasily. I wasn't sure if we need Windows support or not. But it sounds like nobody is really using this platform. So I will just delete the windows part an upload a new change set.
https://codereview.adblockplus.org/29549604/diff/29549714/translations.py File translations.py (right): https://codereview.adblockplus.org/29549604/diff/29549714/translations.py#new... translations.py:86: # ignore system files On 2017/09/19 20:03:54, diegocarloslima wrote: > I think you can change the comment to just 'ignore files' since it will ignore > any file, no matter if it's a system file or a normal one. Acknowledged. https://codereview.adblockplus.org/29549604/diff/29549714/translations.py#new... translations.py:99: return attribute & (win32con.FILE_ATTRIBUTE_HIDDEN | win32con.FILE_ATTRIBUTE_SYSTEM) On 2017/09/19 21:48:04, Vasily Kuznetsov wrote: > Our Python style guide limits lines to 79 characters (following PEP8, which is > more or less of a universal standard). You can reformat this line like this, to > make it compliant: > > return attribute & (win32con.FILE_ATTRIBUTE_HIDDEN | > win32con.FILE_ATTRIBUTE_SYSTEM) Acknowledged.
Hi Jens, I just have one linguistic nit (see below) but otherwise it looks good. Cheers, Vasily https://codereview.adblockplus.org/29549604/diff/29550638/translations.py File translations.py (right): https://codereview.adblockplus.org/29549604/diff/29550638/translations.py#new... translations.py:86: continue; You don't need the semicolon at the end of the line.
https://codereview.adblockplus.org/29549604/diff/29550638/translations.py File translations.py (right): https://codereview.adblockplus.org/29549604/diff/29550638/translations.py#new... translations.py:86: continue; On 2017/09/22 16:27:10, Vasily Kuznetsov wrote: > You don't need the semicolon at the end of the line. Ah, you're right. I know it's not necessary in python but it's just a habit from doing Java all the time.
On 2017/09/26 12:09:18, jens wrote: > https://codereview.adblockplus.org/29549604/diff/29550638/translations.py > File translations.py (right): > > https://codereview.adblockplus.org/29549604/diff/29550638/translations.py#new... > translations.py:86: continue; > On 2017/09/22 16:27:10, Vasily Kuznetsov wrote: > > You don't need the semicolon at the end of the line. > > Ah, you're right. I know it's not necessary in python but it's just a habit from > doing Java all the time. Yeah, I know. Happens to the best of us when one tries to juggle multiple languages. LGTM. 
|
