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

The main loop should not run in the same scope as __init__ #101

Conversation

blacklight
Copy link
Contributor

This package runs the main application directly from __init__.py.

This approach prevents other applications from being able to import the modules exposed by the app, which is a bit of a shame because this package exports a pre-compiled version of the TheengsDecoder library, and it would be nice to import it in other packages without running the whole gateway.

However, without this PR this code would hang:

>>> from TheengsGateway._decoder import decodeBLE as dble

The reason is that run() is executed in the static scope of __init__.py, and therefore the loop is started and joined as soon as the parent module is imported.

This PR addresses this issue by wrapping the main app logic into a main method, and adding a separate __main__.py, as it's common practice in Python.

Description:

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

This package runs the main application directly from __init__.py.

This approach prevents other applications from being able to import the
modules exposed by the app, which is a bit of a shame because this
package exports a pre-compiled version of the TheengsDecoder and it
would be nice to import it in other packages without running the whole
gateway.

However, without this PR this code would hang:

>>> from TheengsGateway._decoder import decodeBLE as dble

The reason is that `run()` is executed in the static scope of
__init__.py, and therefore the loop is started and joined as soon as the
parent module is imported.

This PR addresses this issue by wrapping the main app logic into a
`main` method, and adding a separate __main__.py, as it's common
practice in Python.
@koenvervloesem
Copy link
Member

Sounds reasonable.

That said, is the only reason you need this that you want to use a pre-compiled version of the Theengs Decoder library? It seems a bit of a waste to install the whole gateway package then. Would having a pre-compiled Theengs Decoder package on PyPI help too?

@blacklight
Copy link
Contributor Author

@koenvervloesem thanks for the quick answer!

Would having a pre-compiled Theengs Decoder package on PyPI help too?

Yes, that would definitely help! Ideally I need a thin layer on top of bleak to help me decode the advertised data, and which can be installed as a pip dependency, without going through the manual cmake process currently required by TheengsDecoder.

Installing the gateway just because I need the library is indeed an overkill, but it's the quickest way I found to get the library through a dependency that I could just add to my setup.py.

That being said, I also see the value in the "main-ification" of this module - it's not very "Pythonic" to have a module that automatically starts a synchronous process as soon as you import it.

@koenvervloesem
Copy link
Member

Yes, that would definitely help! Ideally I need a thin layer on top of bleak to help me decode the advertised data, and which can be installed as a pip dependency, without going through the manual cmake process currently required by TheengsDecoder.

We'll work on it. It was already on our roadmap, but just never happened because we haven't seen the decoder being used by outside projects yet.

Just out of curiosity: in which project are you going to use the decoder?

That being said, I also see the value in the "main-ification" of this module - it's not very "Pythonic" to have a module that automatically starts a synchronous process as soon as you import it.

Sure, we just never imagined that anyone would use the gateway in another way than running the command :-) But you're absolutely right, thanks for your PR, we'll review it and this is a welcome improvement of the code!

@blacklight
Copy link
Contributor Author

blacklight commented Feb 18, 2023

Just out of curiosity: in which project are you going to use the decoder?

I'm planning to use it in Platypush - which is a bit like a general-purpose HomeAssistant / Tasker / IFTTT on steroids.

The work that I'm currently doing is part of a larger PR that involves the refactoring of the current integrations into something more general-purpose, with entities discovered dynamically.

I have recently done a similar job when refactoring the Zigbee and Z-Wave integrations, so that they can "scan and figure out" all the compatible entities without need for manual mappings or additional integrations, and I've just started to tackle the same issue for Bluetooth - just to discover that the Bluetooth world is much more complex because every device publishes things in whichever format they like :)

Since I didn't want to reinvent the wheel and do ad-hoc testing and integrations with tens or hundreds of devices, I was looking for a library that could do the job for me (i.e. figure out from the advertised services and data which are the supporter properties and their values), and Theengs seemed to check all the boxes (except for the on-the-fly pip integration).

Of course one option could also be to build a bluetooth integration that requires a Theengs gateway to be running, and Platypush would just subscribe and parse events from the message queue, but I'd rather avoid another layer of indirection when I've already built most of the logic to keep track of the state in the existing integration.

@koenvervloesem koenvervloesem merged commit a053bf5 into theengs:development Feb 20, 2023
@koenvervloesem
Copy link
Member

So thanks again for this PR, it has been merged. If you have further questions about integrating decoder, you can open an issue there. You can expect a package on PyPI soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants