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

Worker class organization #1784

Closed
wants to merge 15 commits into from
Closed

Worker class organization #1784

wants to merge 15 commits into from

Conversation

ccrvlh
Copy link
Collaborator

@ccrvlh ccrvlh commented Feb 1, 2023

Small organization effort to make related methods grouped together: no changes in logic - it should be easy to see the organization with Worker class folded. Was separates "geographically" the work horse related methods (there was a suggestion a while ago to completely separate this). This was built on top of #1753

Command to ignore moved methods (it should be all greyed out): git -c color.diff.newMoved=black -c color.diff.oldMoved=black diff --color-moved=plain --unified=0

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Base: 94.93% // Head: 94.91% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (162f335) compared to base (436250d).
Patch coverage: 88.71% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1784      +/-   ##
==========================================
- Coverage   94.93%   94.91%   -0.03%     
==========================================
  Files          51       51              
  Lines        7740     7744       +4     
==========================================
+ Hits         7348     7350       +2     
- Misses        392      394       +2     
Impacted Files Coverage Δ
rq/worker.py 88.05% <88.59%> (-0.02%) ⬇️
tests/test_worker.py 98.16% <100.00%> (+<0.01%) ⬆️
rq/types.py 83.33% <0.00%> (-2.39%) ⬇️
rq/job.py 97.24% <0.00%> (-0.01%) ⬇️
tests/test_cli.py 98.88% <0.00%> (-0.01%) ⬇️
rq/defaults.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ccrvlh
Copy link
Collaborator Author

ccrvlh commented Feb 4, 2023

@selwin this doesn't make sense after black formatting. Anyways, I would like to hear your opinion on this sort of organization vs the current one - a very similar thing could be adopted in the Queue and the Job that (IMHO) have some methods in not obvious places. Appreciate if you could take a look (ignoring formatting) on this anyways. I'll close it afterwards. Thanks!

@selwin
Copy link
Collaborator

selwin commented Feb 4, 2023

Hey sorry it's very hard for me to spot the changes, even if I compare this branch to the commit before I pulled in the PR with black formatting c2e6d95...162f335 .

Any specific commit I should look at?

@ccrvlh
Copy link
Collaborator Author

ccrvlh commented Feb 4, 2023

No commit - this branch is actually a single commit, the rest of them were already merged. You won't be able to compare properly indeed - but you can just open the worker file, fold it all and just open the Worker class, that way you'll see how it's organized.

@ccrvlh ccrvlh closed this Feb 13, 2023
@ccrvlh ccrvlh deleted the reorder branch March 12, 2023 11:01
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.

None yet

2 participants