Conversation
Codecov Report
@@ Coverage Diff @@
## master #243 +/- ##
==========================================
- Coverage 91.37% 90.42% -0.96%
==========================================
Files 12 12
Lines 580 595 +15
==========================================
+ Hits 530 538 +8
- Misses 50 57 +7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
If I am interpreting the output from the codecov bot correctly, all of the decrease in the measured percentage of code coverage can be attributed to the patch's changes to code that only executes in a Jupyter notebook setting, specifically changes to If desired, I could introduce mocks in an additional unittest to permit this to kind-of run just to get the code coverage percentage higher though I did not see any existing unit tests to do anything similar? I'm not sure how much practical value it would have beyond bumping the percentages? |
till-m
left a comment
till-m
left a comment
There was a problem hiding this comment.
Hi @applio,
thanks for the contribution. Some small remarks, but this looks good to me (code-wise + when running in a terminal), unfortunately I think my ipywidgets are broken locally and I cannot check whether they work properly in a notebook. @nalepae could you check maybe?
I'm not entirely sure what the usecase of multiple progress bars is, when one progress bar represents multiple workers. What is the advantage here compared to using just one progress bar?
The only thing that this is lacking (from my side) is documentation. This does not need to happen in this PR as far as I am concerned, but I would appreciate if you could expand the section here.
If desired, I could introduce mocks in an additional unittest to permit this to kind-of run just to get the code coverage percentage higher though I did not see any existing unit tests to do anything similar? I'm not sure how much practical value it would have beyond bumping the percentages?
I don't think this would be good, it's basically just window dressing if I understand correctly. Unfortunately I think right now there is no way around doing this manually, but then again, with primarily visual features I think one should check them manually anyways.
| nb_requested_workers: int, | ||
| data_type: Type[DataType], | ||
| progress_bars_type: ProgressBarsType, | ||
| max_progress_bars=-1, |
| nb_requested_workers: int, | ||
| data_type: Type[DataType], | ||
| progress_bars_type: ProgressBarsType, | ||
| max_progress_bars=-1, |
|
|
||
| class ProgressBarsConsole(ProgressBars): | ||
| def __init__(self, maxs: List[int], show: bool) -> None: | ||
| def __init__(self, maxs: List[int], show: bool, count=-1) -> None: |
|
|
||
| class ProgressBarsNotebookLab(ProgressBars): | ||
| def __init__(self, maxs: List[int], show: bool) -> None: | ||
| def __init__(self, maxs: List[int], show: bool, count=-1) -> None: |
| show_progress_bars = progress_bars_type != ProgressBarsType.No | ||
|
|
||
| progress_bars = get_progress_bars(progresses_length, show_progress_bars) | ||
| progress_bars = get_progress_bars(progresses_length, show_progress_bars, max_progress_bars) |
There was a problem hiding this comment.
Check line length -- I think this repo (generally) follows the PEP8 rule of max 79 character lines. @nalepae do we have an auto-formatter that we use?
|
Thank you for your well-written PR! However, I do not get exactly the point of having a number of progress bars that is neither I do understand that having like However, I don't really see the added value to have something in between. Can you please explain the rationale a little bit more? |
3 participants
As described in issue #242, this PR proposes adding a way to control the maximum number of progress bars to display that maintains backwards-compatibility with existing code.
Control over the number of progress bars is made through the existing input parameter to
pandarallel.initialize():Expanded testing is provided by both expanding the number of params to the
pytest.fixture,progress_bar()as well as one new specific unit test.