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

Proposition: optional override to is_instance(x,t) to support both types and str in the same function #191

Closed
wants to merge 1 commit into from

Conversation

kessido
Copy link
Contributor

@kessido kessido commented Nov 11, 2020

splitting the is_instance, and is_instance_str seemed like a complicated solution imho, I'm not too keen with the override, but I find it much more appealing in the later uses imo.

Then again, just a proposition.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jph00
Copy link
Member

jph00 commented Nov 13, 2020

I think this is a good idea, but let's change the definition of rinstance, which we already have, instead of replacing isinstance. Does that sound OK?

@kessido
Copy link
Contributor Author

kessido commented Nov 14, 2020

I think this is a good idea, but let's change the definition of rinstance, which we already have, instead of replacing isinstance. Does that sound OK?

Sounds ok
I find it less ram consuming (metaphorically) to add it to the base function as another functionality~

@kessido
Copy link
Contributor Author

kessido commented Nov 14, 2020

I think this is a good idea, but let's change the definition of rinstance, which we already have, instead of replacing isinstance. Does that sound OK?

Another option btw, is to combine both risinstance and isinstance. e.g:
https://github.com/kessido/kesscore/blob/3657275b4e69cf708a5bcdbde6ab7afd583cdda9/kesscore/functional.py#L45
but that just a matter of taste, I find it more appealing to write:
L(1,2,3).map(isinstance(int)).all()

@hamelsmu
Copy link
Member

@kessido where did you end up on this PR? Just want to follow up. I see some discussions about changing rinstance but not sure I see any commits after this. Thanks

@jph00
Copy link
Member

jph00 commented Dec 21, 2020

Since this had gotten rather stale I implemented it myself. risinstance now handles strings and lists of strings. isinstance_str is still there, but just to support array_equal.

@jph00 jph00 closed this Dec 21, 2020
@jph00 jph00 reopened this Dec 21, 2020
@jph00 jph00 added the enhancement New feature or request label Dec 21, 2020
@jph00 jph00 closed this in 4e2b8a3 Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants