Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed support for get_type #104

Merged
merged 1 commit into from
Oct 19, 2021
Merged

Fixed support for get_type #104

merged 1 commit into from
Oct 19, 2021

Conversation

mitar
Copy link
Collaborator

@mitar mitar commented Oct 2, 2021

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

@Stewori
Copy link
Owner

Stewori commented Oct 2, 2021

@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
Copy link
Collaborator Author

mitar commented Oct 3, 2021

Where should the note go?

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

@mitar
Copy link
Collaborator Author

mitar commented Oct 3, 2021

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

@Stewori
Copy link
Owner

Stewori commented Oct 3, 2021

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
Copy link
Collaborator Author

mitar commented Oct 3, 2021

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
Copy link
Owner

Stewori commented Oct 4, 2021

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
Copy link
Collaborator Author

mitar commented Oct 4, 2021

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
Copy link
Owner

Stewori commented Oct 5, 2021

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
Copy link
Owner

Stewori commented Oct 5, 2021

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:

  • get_type overrides all other attempts to get the type, including get_orig_class. This gives maximum control to the user; if wanted, one can retain use of get_orig_class by calling it explicitly from the provided get_type function.
  • the fallback of using __class__ is not totally discarded; maybe it does something good in some cases.

@mitar
Copy link
Collaborator Author

mitar commented Oct 18, 2021

I updated it based on your proposal.

@Stewori
Copy link
Owner

Stewori commented Oct 19, 2021

Thanks!

@Stewori Stewori merged commit 121f9c3 into Stewori:master Oct 19, 2021
@mitar mitar deleted the fix_get_type branch October 21, 2021 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants