Issue 4653 - Add handling for bookmarks to readMetadata

sitescripts/extensions/utils.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 165 matching lines @@
 
         if self.nightlyConfig and not self.nightlyConfig.has_section(self.repositoryName):
             self.nightlyConfig.add_section(self.repositoryName)
 
     def __str__(self):
         """
           Provides a string representation of this configuration
         """
         return self.repositoryName
 
-    def readMetadata(self, version='tip'):
+    def readMetadata(self, version):
         genericFilename = 'metadata'
         filename = '%s.%s' % (genericFilename, self.type)
         files = subprocess.check_output(['hg', '-R', self.repository,
                                          'locate', '-r', version]).splitlines()
 
         if filename not in files:
             # some repositories like those for Android and
             # Internet Explorer don't have metadata files
             if genericFilename not in files:
                 return None
 
             # Fall back to platform-independent metadata file
             filename = genericFilename
 
         command = ['hg', '-R', self.repository, 'cat', '-r', version, os.path.join(self.repository, filename)]
         result = subprocess.check_output(command)
 
         parser = SafeConfigParser()
         parser.readfp(StringIO(result))
 
         return parser
 
     def getDownloads(self):
         metadata = self.readMetadata(self.revision)

[Sebastian Noack, 2016/11/25 13:01:56]

I think this logic belongs into readMetadata(), so that when no arguments are
supplied that it uses self.revision by default. Now, where we have that value,
it seems more expected that it is used by default rather than tip.

[Jon Sonesen, 2016/11/25 13:05:01]

I agree here.

[Sebastian Noack, 2016/11/25 13:40:29]

For reference, we discussed that a little more on IRC, and as it turned out that
there is no place left where readMetadata() is called without argument, we
decided to leave the code here as is, but make the argument mandatory.

         if metadata:
             prefix = metadata.get('general', 'basename')
         else:
             prefix = os.path.basename(os.path.normpath(self.repository))
         prefix += '-'
 
         command = ['hg', 'locate', '-R', self.downloadsRepo, '-r', 'default']
         for filename in subprocess.check_output(command).splitlines():
             if filename.startswith(prefix) and filename.endswith(self.packageSuffix):
                 yield (filename, filename[len(prefix):len(filename) - len(self.packageSuffix)])
@@ skipping 89 matching lines @@
 
 def getDownloadLinks(result):
     """
     gets the download links for all extensions and puts them into the config
     object
     """
     for repo in Configuration.getRepositoryConfigurations():
         try:
             (downloadURL, version) = _getDownloadLink(repo)
             if downloadURL is None:
-                message = 'No download link found for repo: ' + repo

[Sebastian Noack, 2016/11/25 13:01:56]

Any reason you don't pass the message directly to the Exception() constructor,
but assign it to a variable first?

[Jon Sonesen, 2016/11/25 13:05:01]

I had it in an if statement when i was testing it and forgot to change it, will
pass the message directly

-                raise Exception(message)
+                raise Exception('No download link found for repo: ' + repo)
         except:
             traceback.print_exc()
             continue
         if not result.has_section(repo.repositoryName):
             result.add_section(repo.repositoryName)
         result.set(repo.repositoryName, 'downloadURL', downloadURL)
         result.set(repo.repositoryName, 'version', version)
 
         qrcode = _getQRCode(downloadURL)
         if qrcode is not None:
@@ skipping 46 matching lines @@
     if not extensions:
         return
 
     updates = {}
     for extension in extensions:
         updates[extension['basename']] = {
             'url': extension['updateURL'],
             'version': extension['version']
         }
     writeLibabpUpdateManifest(path, updates)