-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improve non-parallel demo #40
Conversation
* Simplify synchronous demo * Update docs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
src/tqdm_publisher/_demo/_server.py
Outdated
This is a class that handles the WebSocket connections and the communication protocol | ||
between the server and the client. | ||
|
||
While we could have implemented this as a function, a class is chosen here to maintain reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not too arduous, could you show me what the functional form of this would look like in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing. Reads much cleaner that way IMO
Four well-documented functions explain exactly what needs to be scoped in what, with all function and variable names explicit in their purpose
* Update _server.py * more keyword arguments; break up docstring; remove unused capture * more keyword arguments --------- Co-authored-by: Garrett Michael Flynn <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
============================================
- Coverage 100.00% 39.77% -60.23%
============================================
Files 3 5 +2
Lines 35 88 +53
============================================
Hits 35 35
- Misses 0 53 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@garrettmflynn I think this can be merged with your approval I may still propose some follow-ups to adjust README and maybe add a new child class to the base that will make injection to downstream libraries simpler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Thank you for your attention on this.
Emphasizing this demo as a more expansive 'how to' by enhancing the readability of all Pythonic parts
Should probably do the same for the script part of the
_client.html
, probably using in-line comments (unless there's a notion of docstrings in HTML?)I did break something as of the latest commit however; the progress bar runs on the console but must not be sending updates properly to the client - can you take a look and see if you can get it working without altering too much of the current nesting structure?
I can see how some of the local scoping was necessary - this was harder to determine from the anonymized callable (
lambda
) representation - but I've tried to denest it and document it as thoroughly as possible