An ability to access PubSubHub from outside Flet app #3446
Conversation
Reviewer's Guide by SourceryThis pull request introduces the ability to access PubSubHub from outside the Flet app by refactoring the way PubSubHub instances are managed and accessed. The changes centralize the PubSubHub management in the FletAppManager class, allowing for more flexible and thread-safe access. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @FeodorFitsner - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| self.__pubsubhubs_lock = threading.Lock() | ||
| self.__pubsubhubs = {} |
There was a problem hiding this comment.
suggestion: Consider using an asyncio.Lock instead of threading.Lock
Since the rest of the code appears to be using asyncio constructs, it might be more consistent and potentially safer to use an asyncio.Lock instead of threading.Lock.
| self.__pubsubhubs_lock = threading.Lock() | |
| self.__pubsubhubs = {} | |
| self.__pubsubhubs_lock = asyncio.Lock() | |
| self.__pubsubhubs = {} |
| def get_pubsubhub( | ||
| self, session_handler, loop: Optional[asyncio.AbstractEventLoop] = None | ||
| ): |
There was a problem hiding this comment.
suggestion: Add type hint for session_handler parameter
To improve code readability and maintainability, consider adding a type hint for the session_handler parameter.
| def get_pubsubhub( | |
| self, session_handler, loop: Optional[asyncio.AbstractEventLoop] = None | |
| ): | |
| def get_pubsubhub( | |
| self, session_handler: SessionHandlerType, loop: Optional[asyncio.AbstractEventLoop] = None | |
| ): |
| psh = self.__pubsubhubs.get(session_handler, None) | ||
| if psh is None: |
There was a problem hiding this comment.
issue (bug_risk): Potential race condition with pubsubhubs dictionary
Although the lock should prevent race conditions, consider using a defaultdict to simplify the logic and reduce the risk of potential issues.
| psh = self.__pubsubhubs.get(session_handler, None) | ||
| if psh is None: | ||
| psh = PubSubHub( | ||
| loop=loop if loop else asyncio.get_running_loop(), |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| loop=loop if loop else asyncio.get_running_loop(), | |
| loop=loop or asyncio.get_running_loop(), |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
* An ability to access PubSubHub from outside Flet app Close flet-dev#3445 * Fix self.__pubsubhubs_lock * Use "or" instead of "if-else"
Close #3445
Summary by Sourcery
This pull request introduces a new feature that allows accessing PubSubHub from outside the Flet app. It also includes a refactoring to centralize the management of PubSubHub instances within FletAppManager, enhancing code maintainability.