Skip to content

style: deal with B024 added to flake8 recently #232

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

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

jiang1997
Copy link
Contributor

@jiang1997 jiang1997 commented Aug 28, 2022

More details about B024 here PyCQA/flake8-bugbear#274

./skywalking/agent/protocol/init.py:22:1: B024 Protocol is an abstract base class, but it has no abstract methods.

Set 'heartbeat', 'report' and 'report_log' as abstract method

./skywalking/trace/span.py:36:1: B024 Span is an abstract base class, but it has no abstract methods.

Prevent base class Span from being directly instantiated by raising error in __new__ with reference to
https://stackoverflow.com/questions/7989042/preventing-a-class-from-direct-instantiation-in-python

./tests/plugin/base.py:35:1: B024 TestPluginBase is an abstract base class

Due to pytest instantiating parent class for searching test functions, TestPluginBase has to be instantiable.

@@ -35,7 +35,7 @@
from skywalking.trace.context import Segment

__started = False
__protocol = Protocol() # type: Protocol
__protocol = None # type: Protocol
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incompatible variable type: __protocol is declared to have type Protocol but is used as type None.


Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@jiang1997 jiang1997 marked this pull request as ready for review August 28, 2022 14:31
):
if cls is Span:
raise TypeError(f"only children of '{cls.__name__}' may be instantiated")
return super.__new__(cls, *args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Superskyyy Superskyyy added the chore Project chores label Aug 30, 2022
@Superskyyy Superskyyy added this to the 1.0.0 milestone Aug 30, 2022
More details about B024 here PyCQA/flake8-bugbear#274

* ./skywalking/agent/protocol/__init__.py:22:1: B024 Protocol is an abstract base class, but it has no abstract methods.
Set 'heartbeat', 'report' and 'report_log' as abstract method

* ./skywalking/trace/span.py:36:1: B024 Span is an abstract base class, but it has no abstract methods.
Prevent base class Span from being directly instantiated by raising error in __new__ with reference to
https://stackoverflow.com/questions/7989042/preventing-a-class-from-direct-instantiation-in-python

* ./tests/plugin/base.py:35:1: B024 TestPluginBase is an abstract base class
Due to pytest instantiating parent class, TestPluginBase has to be instantiable.
@jiang1997
Copy link
Contributor Author

jiang1997 commented Sep 1, 2022

So far, the only problem is that the test of flask uses @runnable, and according to this line,

return self.new_span(parent, Span, op=op, kind=Kind.Local)

Span has to be instantiable too.

Seems to be no alternatives for Span. Should I keep Span unchanged except removing ABC?

@Superskyyy
Copy link
Member

So far, the only problem is that the test of flask uses @runnable, and according to this line,

return self.new_span(parent, Span, op=op, kind=Kind.Local)

Span has to be instantiable too.
Seems to be no alternatives for Span. Should I keep Span unchanged except removing ABC?

Yeah just keep it for now. :)

@Superskyyy
Copy link
Member

LGTM

@Superskyyy Superskyyy self-requested a review September 2, 2022 05:57
@Superskyyy Superskyyy merged commit 91a45ce into apache:master Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Project chores
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants