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

Create public aliases for arguments #6

Closed
KalleDK opened this issue Jul 31, 2020 · 6 comments
Closed

Create public aliases for arguments #6

KalleDK opened this issue Jul 31, 2020 · 6 comments
Labels
invalid This doesn't seem right

Comments

@KalleDK
Copy link

KalleDK commented Jul 31, 2020

The problem I'm trying to solve is having a method that takes an element or elementtree as argument, and forwards that to a XPath object.

x = etree.XPath("//div")

def method(elem):
  // other stuff
  r = x(elem)
  // other stuff

If I do this with fx pylance, I would get the complaint that elem is Unknown. If I try to fix this by adding the type, I would have to write

def method(elem: typing.Union[etree._Element, etree._ElementTree]):

Which of course triggers that the types are private.

For easy fix I would suggest some simple typing aliases

TElement = _Element or something like that (Don't really care about the syntax - more about that the types would be public)

@mvantellingen
Copy link
Collaborator

mvantellingen commented Aug 9, 2020

@KalleDK
Copy link
Author

KalleDK commented Aug 9, 2020

Not if I understand it correctly

As for example parse https://github.com/lxml/lxml-stubs/blob/master/lxml-stubs/etree.pyi#L371 and getnext https://github.com/lxml/lxml-stubs/blob/master/lxml-stubs/etree.pyi#L104 return _ElementTree and _Element along with most of the methods. This would not match ElementBase as it's a subclass of _Element.

def method(elem: etree.ElementBase):
  pass

t = etree.parse(.....)
method(t)

This would say that they don't match.

@scoder
Copy link
Member

scoder commented Aug 9, 2020

You can use ElementBase for that right?

ElementBase is a public subclass of _Element that is provided for users as a base class for their own Element classes. It's not a catch-all base class for Elements.

Which of course triggers that the types are private.

So? A private class does not prevent its objects from being passed into functions. Using a private class as argument type hints seems totally reasonable to me, and the warning that you get seems a false positive.

The convention is lxml is that "private" (starting with an underscore) classes are not meant to be instantiated by users. That doesn't prevent them from being used in other ways, e.g. for typing.

@KalleDK
Copy link
Author

KalleDK commented Aug 9, 2020

So? A private class does not prevent its objects from being passed into functions. Using a private class as argument type hints seems totally reasonable to me, and the warning that you get seems a false positive.

The convention is lxml is that "private" (starting with an underscore) classes are not meant to be instantiated by users. That doesn't prevent them from being used in other ways, e.g. for typing.

I know I can use the private classes for typing, but I would say I would be breaking some kinda python rule if my module uses private classes from another module (even if it's only for typing)

Also if I do use them, pylance ( and other static tools ) will complain that you are using private classes

@scoder
Copy link
Member

scoder commented Aug 10, 2020

static tools will complain that you are using private classes

Then I would argue that those tools need to be fixed. Apparently, it's not enough to warn about imported names that simply start with an underscore, it's also relevant what they are used for. If they are not used at all, that's an "unused import" case. If they are getting called or instantiated or otherwise actively used in code, that's a violation of an API contract. If they are used for isinstance() type checks, that might be ok but has a code smell (lxml provides an iselement() predicate function, for example). If they are used in type hints, that's probably fine.

lxml might be excessive here with its relatively high number of internal classes, but I'm sure it's not the only package where this problem appears. Adding a non-private alias for the _Element class would counter the intention of keeping private classes private.

@scoder scoder added the invalid This doesn't seem right label Nov 14, 2020
@scoder
Copy link
Member

scoder commented Nov 14, 2020

Closing, since it's a third-party issue.

@scoder scoder closed this as completed Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants