Issue 3967 - Properly handle non-existing file-mappings

packager.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 # Note: These are the base functions common to all packagers, the actual
 # packagers are implemented in packagerGecko and packagerChrome.
 
 import sys
 import os
 import re
@@ skipping 75 matching lines @@
         self.process = process
 
     def __setitem__(self, key, value):
         if self.process:
             value = self.process(key, value)
         dict.__setitem__(self, key, value)
 
     def isIncluded(self, relpath):
         return relpath.split('/')[0] in self.includedFiles
 
-    def isignored(self, relpath):

[kzar, 2017/07/17 09:37:40]

Nit: Should it be is_ignored? (Kind of a question for Sebastian / Vasily)

[tlucas, 2017/07/17 12:29:48]

for the sake of readability i would agree. Acknowledged

+    def is_ignored(self, relpath):
         parts = relpath.split('/')
-        return any([part in self.ignoredFiles for part in parts])
+        return any(part in self.ignoredFiles for part in parts)
 
     def read(self, path, relpath='', skip=()):
         if os.path.isdir(path):
             for file in os.listdir(path):
                 name = relpath + ('/' if relpath != '' else '') + file
-                if (name not in skip
-                        and self.isIncluded(name) and not self.isignored(name)):

[kzar, 2017/07/17 09:37:40]

The indentation looks a bit funky here, but I'm rusty with Python. Did you check
that linting passes?

[tlucas, 2017/07/17 12:29:48]

flake8 didn't nag about this at all - and from what i understood, Sebastian
wanted exactly that (PEP-8 adherence)

[kzar, 2017/07/17 12:32:52]

Fair enough, let's see what he thinks.

[Sebastian Noack, 2017/07/17 14:02:31]

The following two styles are acceptable according to PEP-8
(https://www.python.org/dev/peps/pep-0008/#indentation):

  if (this_is_one_thing and
      that_is_another_thing):
      do_something()

  if (this_is_one_thing
          and that_is_another_thing):
      do_something()

I personally, prefer the former, which is also more consistent with what we have
in our existing code base.

Another option is to use some temporary variables in order to avoid wrapping in
the first place:

  included = self.isIncluded(name) and not self.isignored(name)
  if name not in skip and included:
      ...

[tlucas, 2017/07/17 16:34:12]

As discussed, outsourcing the logic to a variable imho seems to be the most
elegant solution for this, Done

+
+                included = self.isIncluded(name) and not self.is_ignored(name)
+                if name not in skip and included:
                     self.read(os.path.join(path, file), name, skip)
         else:
             with open(path, 'rb') as file:
                 if relpath in self:
                     print >>sys.stderr, 'Warning: File %s defined multiple times' % relpath
                 self[relpath] = file.read()
 
     def readMappedFiles(self, mappings):
         for item in mappings:
             target, source = item
 
-            # Adhere to given ignorance

[kzar, 2017/07/17 09:37:40]

I'm not sure this comment adds much, maybe just remove it?

[tlucas, 2017/07/17 12:29:48]

Acknowledged.

-            if '/' in target and self.isignored(target):
+            if '/' in target and self.is_ignored(target):
                 continue
 
             parts = source.split('/')
             path = os.path.join(os.path.dirname(item.source), *parts)
             if os.path.exists(path):
                 self.read(path, target)
             else:
                 print >>sys.stderr, "Warning: Mapped file %s doesn't exist" % source
 
     def preprocess(self, filenames, params={}):
         import jinja2
         env = jinja2.Environment()
 
         for filename in filenames:
             env.autoescape = os.path.splitext(filename)[1].lower() in ('.html', '.xml')
             template = env.from_string(self[filename].decode('utf-8'))
             self[filename] = template.render(params).encode('utf-8')
 
     def zip(self, outFile, sortKey=None, compression=zipfile.ZIP_DEFLATED):
         with zipfile.ZipFile(outFile, 'w', compression) as zf:
             for name in sorted(self, key=sortKey):
                 zf.writestr(name, self[name])
 
     def zipToString(self, sortKey=None):
         buffer = StringIO()
         self.zip(buffer, sortKey=sortKey)
         return buffer.getvalue()