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

Split Synchronous Demos #43

Merged
merged 16 commits into from
Mar 14, 2024
Merged

Split Synchronous Demos #43

merged 16 commits into from
Mar 14, 2024

Conversation

garrettmflynn
Copy link
Collaborator

This PR splits the synchronous demos based on my discussion with @CodyCBakerPhD yesterday.

Limiting the first demo to a single progress bar per client limits the complexity substantially.

Will start this in draft mode so we can have a robust back-and-forth about organization and other aspects of this PR before a full review.

@CodyCBakerPhD
Copy link
Member

In addition to comments in changed lines,

  1. tqdm_publisher single leads to 404 for me
  2. Can you name the CLI command names to demo_single and demo_multiple?

@garrettmflynn
Copy link
Collaborator Author

Any errors in the terminal?

@CodyCBakerPhD
Copy link
Member

tqdm_publisher single leads to 404 for me

The same one on the webpage, 404

Worth noting it sent me to port 1234 not 8000

@garrettmflynn
Copy link
Collaborator Author

Can you try again now that the server functions are run directly?

@garrettmflynn
Copy link
Collaborator Author

garrettmflynn commented Mar 14, 2024

I'm forcing the client to open at 1234 so all the moving parts of this application are properly coordinated. There's some new code to open the webpage since the previous method doesn't allow for modular JavaScript usage (only standalone HTML pages with independent scripts)

@CodyCBakerPhD
Copy link
Member

It opens now, but progress bar itself gives

TypeError: send_message_to_client() takes 0 positional arguments but 2 were given

Are both demos working on your end?

@garrettmflynn
Copy link
Collaborator Author

Ah got it. Fixed, I'd been passing some variables as positional arguments but didn't realize that the preceding * blocked that.

@CodyCBakerPhD
Copy link
Member

Would it be possible to add some test to the left of the button in the multiple demo page to indicate that the button can be clicked multiple times to spawn multiple bars?

@CodyCBakerPhD
Copy link
Member

(Both are working now, thanks)

* enhance documentation; remove remaining anonymous callables; better variable names

* finish docstring
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 6.25000% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 54.54%. Comparing base (388d4be) to head (85e7a6f).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #43       +/-   ##
===========================================
+ Coverage   39.77%   54.54%   +14.77%     
===========================================
  Files           5        4        -1     
  Lines          88       66       -22     
===========================================
+ Hits           35       36        +1     
+ Misses         53       30       -23     
Flag Coverage Δ
unittests 54.54% <6.25%> (+14.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/tqdm_publisher/_publisher.py 100.00% <100.00%> (ø)
...m_publisher/_demos/_demo_command_line_interface.py 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review March 14, 2024 20:50
@CodyCBakerPhD CodyCBakerPhD merged commit e381f16 into main Mar 14, 2024
13 of 14 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the simplified-demos branch March 14, 2024 20:50
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