Skip to content

Add with statement to example in README.md#1435

Merged
felixdivo merged 2 commits into
hardbyte:developfrom
piotrszleg:readme-with
Nov 24, 2022
Merged

Add with statement to example in README.md#1435
felixdivo merged 2 commits into
hardbyte:developfrom
piotrszleg:readme-with

Conversation

@piotrszleg
Copy link
Copy Markdown
Contributor

@piotrszleg piotrszleg commented Nov 15, 2022

Not using with expression in code might cause lack of cleanup and hard to trace errors, also because Bus doesn't override __del__.

So it's a good idea to provide a 100% percent correct example in README for people who don't go into examples folder and read into the code.

Not sure if a sleep call is not needed after the notifier though.

Not using with expression in code might cause lack of cleanup and hard to trace errors, also because Bus doesn't override __del__.
So it's a good idea to provide a 100% percent correct example in README for people who don't go into examples folder and read into the code.
Comment thread README.rst Outdated
@@ -91,21 +91,21 @@ Example usage

# create a bus instance
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
# create a bus instance
# create a bus instance (using a context manager to call `Bus.shutdown()` after use)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a similar comment but with "with" statement instead of context manager, because people might know about it from using files but not know the name "context manager".

@felixdivo
Copy link
Copy Markdown
Collaborator

This is really funny since the docs already give a good example. ^^ Thanks for the contribution!

Do you think we should add a __del__ which calls shutdown()?

@piotrszleg
Copy link
Copy Markdown
Contributor Author

piotrszleg commented Nov 21, 2022

Do you think we should add a __del__ which calls shutdown()?

In this case there should also be a _is_shutdown property to make sure shutdown is not called more than once.

But I think most of users, including me, prefer to shutdown it manually and not depend on garbage collection.

@felixdivo
Copy link
Copy Markdown
Collaborator

Do you think we should add a __del__ which calls shutdown()?

In this case there should also be a _is_shutdown property to make sure shutdown is not called more than once.

But I think most of users, including me, prefer to shutdown it manually and not depend on garbage collection.

Yeah, or the "double calling" should be taken care of by the subclass. You approach is probably simplest.

It was just meant like a "last resort". I think you should never really rely on __del__ being called. But if it exists, it might ensure cleanup at least in a few cases.

@felixdivo
Copy link
Copy Markdown
Collaborator

See #1448 for a discussion on __del__.

@felixdivo felixdivo added this to the Next Release milestone Nov 24, 2022
@felixdivo felixdivo merged commit e7e127a into hardbyte:develop Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants