Hi,
On Mon, Jan 29, 2018 at 01:29:59PM +0100, Markus Koschany wrote:
> [sorry forgot to CC the rest]
>
> Hello,
>
> Am 29.01.2018 um 09:30 schrieb Guido Günther:
> > Hi Markus,
> > thanks for pursuing this! Some minor nitpicks:
> >
> > On Mon, Jan 29, 2018 at 12:11:03AM +0100, Markus Koschany wrote:
> > [..snip..]
> >> + if utils.is_security_update(package, pkgversion):
> >> + if ui.yes_no('Do you want to report a regression
> because of a security update? ',
> >> + 'Yes, please inform the LTS and
> security teams.',
> >> + 'No or I am not sure.', True):
> >> + regex = re.compile('(\+|~)deb(\d+)u(\d+)')
> >> + secversion = regex.search(pkgversion)
> >> + distnumber = secversion.group(2)
> >> + support = 'none'
> >
> > Using
> >
> > support = None
> >
> > is a bit more pythonic.
>
> support = None was part of the very first iterations of this patch but
> the string 'none' is used in distributions.json nowadays. I initialize
> the variable with the same value and then I compare the value in our
> support key. None is not equal to 'none' and caused an error when
> Salvatore changed this value in distributions.json.
>
> >> + email_address = []
> >> + try:
> >> + r =
> requests.get('https://security-tracker.debian.org/tracker/distributions.json',
> >> + timeout=self.options.timeout)
> >
> > This will not catch 404 or similar http status codes. If you don't care
> > about the type of error you can just do …
> >
> > r.raise_for_status()
> >
> > … ff you dont handle the error this …
> >
> >> + data = r.json()
> >
> > … will fail like:
> >
> [...]
> >
> > … The raise_for_status exception is caught here alreadyx since
> > requests.exceptions.HTTPError is a subclass of
> > requests.exceptions.RequestException):
> [...]
>
> I thought requests.exceptions.RequestException is the exception base
> class of Python requests and catches all cases? If not, would
Yes, that catch is already correct (just wanted to point this out). The
r.raise_for_status() is the important part to not trip up r.json().
Cheers,
-- Guido
>
> except (requests.exceptions.RequestException,
> requests.exceptions.HTTPError):
>
> suffice?
>
> > It would also improve readability if this whole code block moved into
> a get_security_contact()
> > function.
>
> Maybe.
>
> Markus
>
>
>
>
