Stewori / pytypes

Typing-toolbox for Python 3 _and_ 2.7 w.r.t. PEP 484.
Apache License 2.0
200 stars 20 forks source link

Fixed support for get_type #104

Closed mitar closed 2 years ago

mitar commented 2 years ago

We do not want to use __class__ because then get_type is never called.

Stewori commented 2 years ago

@mitar Thanks for making PR! Would you drop a (small) note what this fixes? Given that Travis-tests don't run any more, did you check this passes tests?

mitar commented 2 years ago

Where should the note go?

Oh, why Travis does not run anymore? You disabled it? Or something else?

mitar commented 2 years ago

I tested on py27, py36 and py38. I didn't test on pypy3, py34, py35.

Stewori commented 2 years ago

I tested on py27, py36 and py38. I didn't test on pypy3, py34, py35.

Okay, that sounds good. I would like it to work on 3.5 for one more release but I will check that out.

Travis

AFAIK they changed policy and are not available any more (for free). I think most projects use a github-actions-based alternative now but I don't have time to figure that out for pytypes in near future. So for now it relies on local tests at home.

Where should the note go?

Oh, just here in the discussion. The PR kind of lacks context as it doesn't refer to an issue. You didn't fix that line for its own sake I suppose. Does it fix or help with #103 ?

mitar commented 2 years ago

AFAIK they changed policy and are not available any more (for free). I think most projects use a github-actions-based alternative now but I don't have time to figure that out for pytypes in near future. So for now it relies on local tests at home.

I see. I will see if I can help with that. If it will help you get to fix issues in #103 sooner. :-)

Oh, just here in the discussion. The PR kind of lacks context as it doesn't refer to an issue. You didn't fix that line for its own sake I suppose. Does it fix or help with #103 ?

No, it is unrelated to #103. I thought of filling an issue but then I realized the fix is so simple I just made the MR. I thought I explained it well enough:

We do not want to use __class__ because then get_type is never called.

So if get_orig_class has the second argument True, then it never raises AttributeError, but returns the __class__ value (defectively doing the same type would be doing). This means the try/except around the call never really gets to except case and never calls get_type. So this is why we have to pass False. If you cannot get original class, abort, and leave to get_type to figure out the type.

Stewori commented 2 years ago

Okay. Did you stumble over a concrete failure of _deep_type caused by this flaw? Sure, one can probably construct one, but don't bother with that. I am just interested whether how this manifests on _deep_type caller level if you have an example readily at hand. I will accept this PR regardless, it's just for curiosity or maybe for writing a test.

mitar commented 2 years ago

Ah, I use that here. Primitives are like functions, so in most cases one passes values as hyper-parameters to other functions, so I do regular type checking, but you can also pass a function itself to another function, and you pass that as a class, not an instance, so I have to check that as subclass check. Slightly misusing things, but easiest to implement.

Stewori commented 2 years ago

Okay, I see. Thanks. I had forgotten and overlooked that get_type is something one supplies to the _deep_type call. Now the fix makes sense to me.

Stewori commented 2 years ago

I gave it another thought: Probably get_type should actually be preferred if provided (i.e. is not None). What do you think of this solution:

    if get_type is not None:
        res = get_type(obj)
    else:
        get_type = type
        try:
            res = get_orig_class(obj, True)
        except AttributeError:
            res = get_type(obj)

If get_type is not used in the remaining function, this can be simplified to

    if get_type is not None:
        res = get_type(obj)
    else:
        try:
            res = get_orig_class(obj, True)
        except AttributeError:
            res = type(obj)

Advantages:

mitar commented 2 years ago

I updated it based on your proposal.

Stewori commented 2 years ago

Thanks!