[mozprofile] AddonManager.addon_details() should throw an AddonFormatError for not well-formed manifests

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
As mentioned by Jeff on bug 931188 the addon_detail() method should throw a more meaningful exception if something goes wrong when retrieving the details:

This is from is_addon():

> > +        try:
> > +            details = self.addon_details(addon_path)
> > +            return details.get('id') is not None
> > +        except:
> 
> Bare except is probably fine here, but always makes me nervous of covering
> up unintended errors.
(Assignee)

Updated

4 years ago
Summary: [mozprofile] AddonManager.addon_details() should throw an AddonFormatException if failures occur → [mozprofile] AddonManager.addon_details() should throw an AddonFormatError for not well-formed manifests
(Assignee)

Comment 1

4 years ago
Created attachment 824321 [details] [diff] [review]
Patch v1
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #824321 - Flags: review?(jhammel)

Comment 2

4 years ago
Comment on attachment 824321 [details] [diff] [review]
Patch v1

Review of attachment 824321 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except for the extraneous __init__ in the exception.  Please remove.

::: mozprofile/mozprofile/addons.py
@@ +19,5 @@
>  
>  
> +class AddonFormatError(Exception):
> +    def __init__(self, message):
> +        Exception.__init__(self, message)

There's really no need for this __init__ function.  Please remove and please add a docstring (which both explains what it is as well as is less ugly than `pass`)
Attachment #824321 - Flags: review?(jhammel) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 824707 [details] [diff] [review]
Patch v2

I assume that's better now! :)
Attachment #824321 - Attachment is obsolete: true
Attachment #824707 - Flags: review?(jhammel)
(Assignee)

Comment 4

4 years ago
Comment on attachment 824707 [details] [diff] [review]
Patch v2

Review of attachment 824707 [details] [diff] [review]:
-----------------------------------------------------------------

Moving to Andrew given that Jeff doesn't seem to be around today.
Attachment #824707 - Flags: review?(jhammel) → review?(ahalberstadt)
Comment on attachment 824707 [details] [diff] [review]
Patch v2

Review of attachment 824707 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I would probably call it malformed as opposed to not_wellformed, but this is fine.
Attachment #824707 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 6

4 years ago
Wellformed is the term if an XML has the right syntax. So the opposite is not wellformed. I don't think that malformed here is a good alternative. At least all parsers also show not wellformed.

Landed as:
https://github.com/mozilla/mozbase/commit/bfa758c70eb41f5259d496629b797e95becf8fd0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 7

4 years ago
Comment on attachment 824707 [details] [diff] [review]
Patch v2

Review of attachment 824707 [details] [diff] [review]:
-----------------------------------------------------------------

Much nicer, thanks!
You need to log in before you can comment on or make changes to this bug.