Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29340721: Issue 3967 - Create directories for mapped files (Closed)

Created:
April 21, 2016, 3:37 p.m. by kzar
Modified:
Nov. 17, 2016, 11:41 a.m.
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 3967 - Create directories for mapped files

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -8 lines) Patch
M packager.py View 2 chunks +15 lines, -8 lines 3 comments Download

Messages

Total messages: 2
kzar
Patch Set 1
April 21, 2016, 3:39 p.m. (2016-04-21 15:39:41 UTC) #1
Sebastian Noack
May 12, 2016, noon (2016-05-12 12:00:24 UTC) #2
https://codereview.adblockplus.org/29340721/diff/29340722/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29340721/diff/29340722/packager.py#newcode99
packager.py:99: def isIgnored(self, relpath):
Nit: New names should be lowercase.

https://codereview.adblockplus.org/29340721/diff/29340722/packager.py#newcode111
packager.py:111: self.isIncluded(name) and not self.isIgnored(name)):
Nit: The indentation here isn't compliant with PEP-8, and not very readable,
since the last line of the condition is on the same indentation level as the
block body.

One way to fix that would be:

  if name not in skip and (self.isIncluded(name) and not
                           self.isIgnored(name)):

Or alternatively, you could use a temporary variable.

https://codereview.adblockplus.org/29340721/diff/29340722/packager.py#newcode129
packager.py:129: self.includedFiles.add(target.split('/')[0])
Why is it necessary to add it add the directory to includedFiles? It seems if we
just go ahead if the file isn't ignored everything would already work as
intended.

Powered by Google App Engine
This is Rietveld