Skip to content

Conversation

anton-ptashnik
Copy link
Contributor

Summary of proposed changes:

  • adapt API for async environment

For now the API is implemented based on async bleak and wrapped in sync Python functions, spawning additional threads per each connected device. While it may be convenient for simple scripts and demo purposes, it makes difficult to use the API in purely async scripts leveraging Python asyncio since calling any sync function blocks an asyncio loop.
The PR removes intermediate sync wrappers thus making API easy to use in async environment.

  • make the API deployment ready

Python library code supposed to be used by others usually gets published to a package registry, PyPI for Python. It makes easy to locate and install a package with a single command: pip install godice
The PR provides a package metadata file used for publishing a package to the PyPI.

  • avoid redundant shell type checks each time a notification is received

For each notification received, a check on dice (shell) type is done to decide how to parse a rolled number. It is not required since a shell type is not going to change for each dice flip.
The PR proposes to pick a shell type per a Dice object on init, then the way notifications are parsed is setup for the object according to a passed shell type.


[project]
name = "godice"
authors = [{name = "Particula", email = "iavtomator@gmail.com"}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo - change email once provided

- fix package version
- add/update docs
@anton-ptashnik
Copy link
Contributor Author

@ParticulaCode @AlexWorland @Geek5510 take a look please

@Geek5510
Copy link
Collaborator

Geek5510 commented Sep 6, 2023

@anton-ptashnik Hi Anton! First of all, thank you very much for your contributions! Overall, the code seems excellent, and we appreciate you taking the time to work on it. We have two requests we'd like to make before we implement them.

  • The demo script should be more documented to help new users.
  • Correct us if we're wrong, but it seems that it is not possible to switch a die's shell once it's connected, which is an intended feature of the API, and we would appreciate it if you'd implement it or give us an example of how it is possible.
    Thanks!

@anton-ptashnik
Copy link
Contributor Author

Correct us if we're wrong, but it seems that it is not possible to switch a die's shell once it's connected

That's a good question that directed me towards a change. Initially I made a shell to be setup on Dice object creation, but this forces us to create another object when we change a shell and want to receive number updates for a new shell. This leads to redundant device reconnection since connection is handled per an object.
I added set_shell function to allow changing a shell for an existing Dice

The demo script should be more documented to help new users.

Done. I added more clarifications to demo.py, hope it became easier to understand now.

Please review and tell if any other changes needed.

@Geek5510
Copy link
Collaborator

Geek5510 commented Sep 7, 2023

Amazing work! We'll do some testing on our side and approve it once it's done. Thank you!

@Geek5510 Geek5510 merged commit 347052b into ParticulaCode:main Sep 11, 2023
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