mozcrash has functions that should live in mozfile

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jeff Hammel, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
mozcrash has a function, extractAll, that we already have in mozfile:

https://github.com/mozilla/mozbase/blob/master/mozfile/mozfile/mozfile.py#L48
https://github.com/mozilla/mozbase/blob/master/mozcrash/mozcrash/mozcrash.py#L21

mozcrash should depend on mozfile and use the version in there.

In addition, mozcrash has a function is_url:

https://github.com/mozilla/mozbase/blob/master/mozcrash/mozcrash/mozcrash.py#L10

This should probably be put in mozfile since we use this all over the
place in various forms (usually i just do `'://' in thing`).

Both packages should be version bumped and re-released to pypi
(Reporter)

Comment 1

5 years ago
Created attachment 700140 [details] [diff] [review]
fix

following the versions of mozfile and mozcrash should be bumped
Attachment #700140 - Flags: review?(wlachance)
Comment on attachment 700140 [details] [diff] [review]
fix

LGTM except:

>diff --git a/mozcrash/mozcrash/mozcrash.py b/mozcrash/mozcrash/mozcrash.py
>index f22e06e..35fca1c 100644
>+def is_url(thing):
>+    """
>+    Return True if thing looks like a URL.
>+    """
>+    # We want to download URLs like http://... but not Windows paths like c:\...

This comment doesn't really make sense for a generic function. Could you put it in the relevant spot in mozcrash?
Attachment #700140 - Flags: review?(wlachance) → review+
(Reporter)

Comment 3

5 years ago
(In reply to William Lachance (:wlach) from comment #2)
> Comment on attachment 700140 [details] [diff] [review]
> fix
> 
> LGTM except:
> 
> >diff --git a/mozcrash/mozcrash/mozcrash.py b/mozcrash/mozcrash/mozcrash.py
> >index f22e06e..35fca1c 100644
> >+def is_url(thing):
> >+    """
> >+    Return True if thing looks like a URL.
> >+    """
> >+    # We want to download URLs like http://... but not Windows paths like c:\...
> 
> This comment doesn't really make sense for a generic function. Could you put
> it in the relevant spot in mozcrash?

Sure, sgtm
(Reporter)

Comment 4

5 years ago
pushed: https://github.com/mozilla/mozbase/commit/47ea9337f311d517163082b37ce756d784ab3e2c
(Reporter)

Updated

5 years ago
Blocks: 829705
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.