-
Notifications
You must be signed in to change notification settings - Fork 343
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
utils.network.hosts: Make subclass of Host instantiable #5714
Conversation
The changes look good to me, however, I'd prefer a clearer description in commit summary and message. |
avocado/utils/network/hosts.py
Outdated
@@ -48,7 +48,7 @@ class Host: | |||
""" | |||
|
|||
def __init__(self, host): | |||
if isinstance(self, Host): | |||
if type(self) == Host: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, there is another approach to avoid performing this check by invoking type
directly, that is we could make the Host
be an abstract class with its __init__
method being an abstract method.
$ cat /tmp/t.py
from abc import ABCMeta, abstractmethod
class Foo(metaclass=ABCMeta):
@abstractmethod
def __init__(self, host):
self.host = host
class Bar(Foo):
def __init__(self, host='localhost'):
super().__init__(host)
b = Bar()
print(b.host)
f = Foo('localhost')
$ python3 /tmp/t.py
localhost
Traceback (most recent call last):
File "/tmp/t.py", line 19, in <module>
f = Foo('localhost')
^^^^^^^^^^^^^^^^
TypeError: Can't instantiate abstract class Foo with abstract method __init__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hello @clebergnu Could you please help me review this patch? Thanks in advance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test Pass: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @clebergnu Could you please help me review this patch? Thanks in advance.
@yanglei-rh the change itself looks good. I apologize introducing the breakage.
In order to silence pylint, and make CI checks pass, please add:
diff --git a/avocado/utils/network/hosts.py b/avocado/utils/network/hosts.py
index 0a1c2add3..7d61ad640 100644
--- a/avocado/utils/network/hosts.py
+++ b/avocado/utils/network/hosts.py
@@ -48,7 +48,7 @@ class Host:
"""
def __init__(self, host):
- if type(self) == Host:
+ if type(self) == Host: # pylint: disable=C0123
raise TypeError("Host class should not be instantiated")
self.host = host
Thanks for your help review. After silence pylint, CI checks pass. |
Hi @luckyh Could you please help me review it again and merge this MR? Thanks a lot. |
@yanglei-rh can you please address CI issue , as have space around #pylint comment |
A regression bug was introduced by 94fb7a8,which results Host's subclass LocalHost can not be instantiated. Existing instantiation methods the same as "localhost = LocalHost()" will no longer fail after changed. Signed-off-by: Lei Yang <[email protected]>
Hi @PraveenPenguin Thanks for your reminder, this problem has been fixed, please help review it again, thanks a lot. |
A regression bug was introduced by 94fb7a8,which results Host's subclass LocalHost can not be instantiated. Existing instantiation methods the same as "localhost = LocalHost()" will no longer fail after changed.
ID:2219366
Signed-off-by: Lei Yang [email protected]