Skip to content

Conversation

@lpuglia
Copy link

@lpuglia lpuglia commented May 23, 2023

Not sure why ApolloBian didn't reply further to the PR(#203) but i'm here to make sure this change is merged.

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #239 (545ac80) into master (46fc0e5) will decrease coverage by 0.83%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
- Coverage   91.37%   90.55%   -0.83%     
==========================================
  Files          12       12              
  Lines         580      593      +13     
==========================================
+ Hits          530      537       +7     
- Misses         50       56       +6     
Impacted Files Coverage Δ
pandarallel/progress_bars.py 72.93% <53.84%> (-2.48%) ⬇️
pandarallel/core.py 91.09% <100.00%> (+0.09%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@till-m
Copy link
Collaborator

till-m commented May 23, 2023

Hi @lpuglia, thanks for taking care of this. Could you also make sure this change is covered by adding a fixture for the single bar option (see e.g. this)?

@lpuglia
Copy link
Author

lpuglia commented May 23, 2023

@till-m should I add tests for codecov?

@till-m
Copy link
Collaborator

till-m commented May 23, 2023

The uncovered lines seem to only relate to the use in notebooks, I don't know any convenient way to test them. If you have something, feel free to let me know. I will try to test them manually by tomorrow so that I can merge this with no further delay.

@till-m till-m requested review from nalepae and till-m May 23, 2023 11:26
Copy link
Collaborator

@till-m till-m left a comment

Choose a reason for hiding this comment

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

Hi @lpuglia,

I tested it, and it works, so the PR looks good in general to me.

Two small changes:
I think the code should either use single_bar or single_progress_bar, not both. And I would also ask you to briefly document the functionality in the user guide.

@lpuglia
Copy link
Author

lpuglia commented Jun 22, 2023

@till-m done.

By the way, it looks fine in both ipywidget
image
and text mode:
image

@till-m
Copy link
Collaborator

till-m commented Jun 26, 2023

Hi @lpuglia,

thanks for changing the function argument name. Could you also add a small line in the documentation, specifically here updating the function parameter description?

if use_memory_fs
else parallelize_with_pipe
)
parallelize = partial(parallelize, single_bar=single_bar)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO instead it would be better to supply the argument in lines 528 onward as it is currently done with the arguments of parallelize. @nalepae any opinion?

@singhsidhukuldeep
Copy link

This is feature is so important that this PR almost brought a tear in my eyes
Please merge this friends @lpuglia @till-m @ApolloBian @synapticarbors @bra-fsn @nalepae

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.

4 participants