1354012 - nice_type_name() prints "basestring" instead of "string"

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Arjun Nair]

Hey,

I'd like to work on this. Pretty new to open source and mozilla though!

Comment 2

[Arjun Nair]

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

Comment 3

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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

Comment 5

[Arjun Nair]

(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.

Comment 6

[Arjun Nair]

Attachment: 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

Comment 7

[Arjun Nair]

Patch posted.

Comment 8

[Georg Fritzsche [:gfritzsche]]

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.

Comment 9

[Arjun Nair]

(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.

Comment 10

[Arjun Nair]

Attachment: Fixed nice_type_name function always returning basestring

Changed isinstance function to issubclass function since the nice_type_name function takes a type object as its input

Comment 11

[Arjun Nair]

Added a new patch instead of amending the old one.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8856794 [details] [diff] [review]
Fixed nice_type_name function always returning basestring

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

Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea159b50362
Fixed nice_type_name function always returning basestring. r=gfritzsche

Comment 14

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/0ea159b50362