Code review comment for lp:~malizor/update-manager/ppa-changelogs

Thanks for your comments so far.
I will try to adress them as soon as I'm back in front of a computer, which should be next week.

Le 15/06/16 02:34 Colin Watson a écrit :

Review: Needs Fixing

I would honestly just depend on python3-launchpadlib. Yes, we don't have anything in the desktop requiring it right now, but that isn't a strong reason not to add it; and without it, most people will continue not to see PPA changelogs, which seems more compelling to me.

Other than that, this seems like a good start and the use of the LP API seems mostly right, but there are a few details to fix up. Thanks!

Diff comments:

> === modified file 'UpdateManager/Core/MyCache.py'
> --- UpdateManager/Core/MyCache.py 2014-06-26 07:03:08 +0000
> +++ UpdateManager/Core/MyCache.py 2016-06-10 23:11:06 +0000
> @@ -268,6 +274,52 @@
> alllines = alllines + line
> return alllines
>
> + def _extract_ppa_changelog_uri(self, name):
> + """Return the changelog URI from the Launchpad API
> +
> + Return None in case of an error.
> + """
> + if Launchpad is None:
> + logging.warning("Please install 'python3-launchpadlib' to enable "
> + "changelog retrieval for PPAs.")
> + return
> + cdt = self[name].candidate
> + for uri in cdt.uris:
> + match = re.search('http.*/(.*)/(.*)/ubuntu/.*', uri)

This should use urllib.parse as the first step, and should only do anything if the hostname is ppa.launchpad.net.

> + if match is not None:
> + user, ppa = match.group(1), match.group(2)
> + break
> + else:
> + logging.error('Unable to extract the changelog from the PPA')
> + return
> +
> + # Login on launchpad if we are not already
> + if self.launchpad is None:
> + try:
> + self.launchpad = Launchpad.login_anonymously('update-manager',
> + 'production',
> + version='devel')
> + except:

Never use bare except. To catch everything that's reasonable to catch in most cases, use "except Exception:".

> + logging.exception("Unable to connect to Launchpad to retrieve "
> + "the changelog")
> + return
> +
> + archive = self.launchpad.archives.getByReference(
> + reference='~%s/ubuntu/%s' % (user, ppa)
> + )
> + if archive is None:
> + logging.error('Unable to extract the changelog from the PPA')
> + return
> +
> + spph = archive.getPublishedSources(source_name=cdt.source_name,
> + exact_match=True,
> + version=cdt.version)

It would be more conventional to use "spphs" given that this is plural.

> + if not spph:
> + logging.error('Unable to extract the changelog from the PPA')
> + return
> +
> + return spph[0].changelogUrl()

Why would a KeyError be relevant? Testing that the collection is non-empty should be enough to ensure that spphs[0] works, and there's no reason to suppose that .changelogUrl() will raise a KeyError. It may be worth catching HTTP exceptions (I forget the exact details), though, as temporary Launchpad outages shouldn't cause update-manager to explode.

> +
> def _guess_third_party_changelogs_uri_by_source(self, name):
> pkg = self[name]
> deb_uri = pkg.candidate.uri
> @@ -314,14 +366,21 @@
> if news:
> self.all_news[name] = news
>
> - def _fetch_changelog_for_third_party_package(self, name):
> + def _fetch_changelog_for_third_party_package(self, name, origins):
> + # Special case for PPAs
> + changelogs_uri_ppa = None
> + for origin in origins:
> + if origin.origin.startswith('LP-PPA-'):
> + changelogs_uri_ppa = self._extract_ppa_changelog_uri(name)

You should add "break" after this; further iterations of the loop won't do anything different.

> # Try non official changelog location
> changelogs_uri_binary = \
> self._guess_third_party_changelogs_uri_by_binary(name)
> changelogs_uri_source = \
> self._guess_third_party_changelogs_uri_by_source(name)
> error_message = ""
> - for changelogs_uri in [changelogs_uri_binary, changelogs_uri_source]:
> + for changelogs_uri in [changelogs_uri_ppa,
> + changelogs_uri_binary,
> + changelogs_uri_source]:
> if changelogs_uri:
> try:
> changelog = self._get_changelog_or_news(

--
https://code.launchpad.net/~malizor/update-manager/ppa-changelogs/+merge/297119

You are the owner of lp:~malizor/update-manager/ppa-changelogs.