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

Update tqdm.py #44

Closed
wants to merge 7 commits into from
Closed

Update tqdm.py #44

wants to merge 7 commits into from

Conversation

formacube
Copy link

add possibility to cumulate or not progress to the parent task progress if parent_task_id is provided

add possibility to cumulate or not progress to the parent task progress if parent_task_id is provided
Clarify reporting with sub-tasks and document new option linked to fix#43
@jedie
Copy link
Collaborator

jedie commented Aug 20, 2021

Thanks for contribution...

But why do we need cumulate_w_parent_progress ???
You can pass parent_task_id or not to update the main task or not.

So i assume this is double code for the same, isn't it?

Expand the README is fine... On the other hand, this has a general problem: it is not guaranteed that the code works, because it is untested. Actually, it is better to add example code and refer to it. Because they can be added to the tests.

Maybe we should enhance https://github.com/boxine/django-huey-monitor/blob/master/huey_monitor_tests/test_app/tasks.py and add more DocStrings there and make more links from README to it?

@formacube
Copy link
Author

formacube commented Aug 20, 2021

I implemented it this way with cumulate_w_parent_progress=True as default parameter so that it is directly compatible with all code written so far by the current users.

For me, from a conceptual point of view, we should have cumulate_w_parent_progress=False as default, as there is no reason why a sub-task counter is in the same order of magnitude as the main-task counter, and why if progress from the subtask is increased by 1, progress of main task will be increased by 1.

In my different types of application, the progress counters of the main task correspond to the number of sub-tasks to be run.
So each time a sub-task is completed, main task-counter has to be increased by 1.

But on the other side, as I told you when I opened the issue, to be useful, the update_dt of the main task has to be updated each time a sub-task updates its progress.

Essentially because a sub-task can take 10h or 24h to complete.

Last month, I had a process which was not showing any progress for 3 weeks.
I had no clue what was going on and as those processes can take a lot of time, I was hesitant to stop and relaunch it.

It made me decide to invest more effort into designing a more efficient progress monitoring of my tasks.

For the testing, I never had professional training on it and I don't know how you work, so anything is fine by me, and I'll be glad to learn how you handle it and integrate your best practices into the ways I work.

I'm not a professional IT. Just an advanced business user who learns by doing and solving his own business isues

huey_monitor/tqdm.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@formacube
Copy link
Author

I implemented your comments.
it should be ok now

huey_monitor/tqdm.py Outdated Show resolved Hide resolved
@jedie
Copy link
Collaborator

jedie commented Nov 25, 2021

@formacube linting failes, see: https://github.com/boxine/django-huey-monitor/runs/4324397296?check_suite_focus=true#step:9:24

You can run tests and linting local with:

make pytest
make lint

Helpfull is also: make fix-code-style ;)

that's a bit stupid if you ask me. but if it pleases you and finally makes this available... I've done it

For such trivial stuff which are not even errors but more your own preferences,
it would take much less time and effort for you to put it directly to your liking.
No real added value to involve me
@formacube
Copy link
Author

linting should be ok and in line with your personal perferences.

I use double line breaks to make code reading & structure easier. That's my preference.
But you are the code owner, so just put it to your preference.

No need to involve me in the future if you prefer lines of 119 characters or if you don't like 2 lines break in a row.

@formacube formacube requested a review from jedie December 10, 2021 13:02
@formacube formacube marked this pull request as draft December 24, 2021 14:41
Copy link
Author

@formacube formacube left a comment

Choose a reason for hiding this comment

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

ok for me

@formacube formacube marked this pull request as ready for review December 24, 2021 14:42
@formacube
Copy link
Author

Can you go on with this ?
I did the cosmetic changes you requested 1 month ago

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2021

Codecov Report

Merging #44 (2d65113) into master (f44622b) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   83.29%   83.46%   +0.17%     
==========================================
  Files          35       35              
  Lines         778      786       +8     
==========================================
+ Hits          648      656       +8     
  Misses        130      130              
Impacted Files Coverage Δ
huey_monitor/tqdm.py 100.00% <100.00%> (ø)
huey_monitor/__init__.py 100.00% <0.00%> (ø)
huey_monitor/humanize.py 90.90% <0.00%> (+2.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f44622b...2d65113. Read the comment docs.

other linting changes (extra whitespace ... no comment !!!!)
Copy link
Author

@formacube formacube left a comment

Choose a reason for hiding this comment

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

ok, fixed 2 other whitespaces ...
hop I will not spend my life there chasing whitespaces !

@formacube formacube marked this pull request as draft December 24, 2021 16:05
@formacube formacube marked this pull request as ready for review December 24, 2021 16:05
@jedie
Copy link
Collaborator

jedie commented Jan 3, 2022

It's green... But i think about to refactor this code part, see: #57

So this PR is useless :(

@jedie
Copy link
Collaborator

jedie commented Jan 3, 2022

I close this in favour of a complete rewrite here: #58

@jedie jedie closed this Jan 3, 2022
jedie pushed a commit that referenced this pull request Jan 7, 2022
Use the code from #44 but without the big README
change, because `cumulate2parents` is already deprecated when it is introduced, see:
*
#60
* #58
@jedie
Copy link
Collaborator

jedie commented Jan 7, 2022

Implement the idea with #61

jedie pushed a commit that referenced this pull request Jan 7, 2022
Use the code from #44 but without the big README
change, because `cumulate2parents` is already deprecated when it is introduced, see:
*
#60
* #58
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.

3 participants