Closed
Bug 1354012
Opened 7 years ago
Closed 7 years ago
nice_type_name() prints "basestring" instead of "string"
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: a26nair, Mentored)
Details
(Whiteboard: [measurement:client] [lang=python])
Attachments
(1 file, 1 obsolete file)
838 bytes,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
STR: - open toolkit/components/telemetry/Events.yaml - to the first "record_in_processes" array, add a number 1 - run: mach build Expected failure message: > ValueError: navigation#search: failed type check for record_in_processes - expected list value type string, got int Actual error message: > ValueError: navigation#search: failed type check for record_in_processes - expected list value type basestring, got int I think this needs fixing in the function nice_type_name() in parse_events.py or its callers.
Reporter | ||
Updated•7 years ago
|
Whiteboard: [measurement:client] → [measurement:client] [lang=python]
Assignee | ||
Comment 1•7 years ago
|
||
Hey, I'd like to work on this. Pretty new to open source and mozilla though!
Assignee | ||
Comment 2•7 years ago
|
||
Figured out the bug. def nice_type_name(t): if isinstance(t, basestring): return "string" return t.__name__ The nice_type_name function takes in a type object with __name__ "basestring" as input. So, a type object is clearly not an instance of basestring, it's an instance of the type class. We will never enter the if block inside the nice_type_name function. A crude solution would be to check if t.__name__ is equal to "basestring". Though there's probably a better way. Would like to know your opinion. -- Arjun
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Arjun Nair from comment #2) > A crude solution would be to check if t.__name__ is equal to "basestring". > Though there's probably a better way. The diagnosis looks good, thanks. The problem with the crude solution is that `basestring` is the base class for two classes we can receive: `str` and `unicode`. Given that we pass types, not instances, we could probably use `issubclass()` here.
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Arjun Nair from comment #1) > Hey, > > I'd like to work on this. Pretty new to open source and mozilla though! Hey, welcome! I'm assigning this bug to you. Our #introduction channel can help with getting started with working on Firefox. Once you confirmed the changes work as per comment 0, you can: - Run the Python linter: mach lint -l flake8 toolkit/components/telemetry - Run the tests, to make sure nothing changed: mach test toolkit/components/telemetry/tests/unit - Submit a patch to this bug: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch
Assignee: nobody → a26nair
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #3) > (In reply to Arjun Nair from comment #2) > > A crude solution would be to check if t.__name__ is equal to "basestring". > > Though there's probably a better way. > > The diagnosis looks good, thanks. > The problem with the crude solution is that `basestring` is the base class > for two classes we can receive: > `str` and `unicode`. > > Given that we pass types, not instances, we could probably use > `issubclass()` here. Oh neat! I didn't know about issubclass. It's quite late here so I'll submit the patch tomorrow.
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Patch posted.
Assignee | ||
Updated•7 years ago
|
Attachment #8856076 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 8•7 years ago
|
||
Comment on attachment 8856076 [details] [diff] [review] The nice_type_name function in parse_events.py took a type object as an input. The check isinstance(t, basestring) was failing because t was a type object and not an instance of the basestring class Review of attachment 8856076 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, i like seeing simple fixes! Can you still change the commit message a bit to be more succinct? You can write more details after an empty line, but the message on the first line should be short. E.g. something like: > Bug XXX - Fix nice_type_name() not printing nice string names. > > ... more details.
Attachment #8856076 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #8) > Comment on attachment 8856076 [details] [diff] [review] > The nice_type_name function in parse_events.py took a type object as an > input. The check isinstance(t, basestring) was failing because t was a type > object and not an instance of the basestring class > > Review of attachment 8856076 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks, i like seeing simple fixes! > > Can you still change the commit message a bit to be more succinct? > You can write more details after an empty line, but the message on the first > line should be short. E.g. something like: > > Bug XXX - Fix nice_type_name() not printing nice string names. > > > > ... more details. Sure.
Assignee | ||
Comment 10•7 years ago
|
||
Changed isinstance function to issubclass function since the nice_type_name function takes a type object as its input
Assignee | ||
Updated•7 years ago
|
Attachment #8856794 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 11•7 years ago
|
||
Added a new patch instead of amending the old one.
Reporter | ||
Updated•7 years ago
|
Attachment #8856076 -
Attachment is obsolete: true
Reporter | ||
Comment 12•7 years ago
|
||
Comment on attachment 8856794 [details] [diff] [review] Fixed nice_type_name function always returning basestring Review of attachment 8856794 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8856794 -
Flags: review?(gfritzsche) → review+
Reporter | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea159b50362 Fixed nice_type_name function always returning basestring. r=gfritzsche
Keywords: checkin-needed
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ea159b50362
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•