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.

Sign up for GitHub

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

Jump to bottom

Reverted PR #7814 & minor code improvement #9494

Merged
merged 19 commits into from
Feb 13, 2025
Merged

Reverted PR #7814 & minor code improvement #9494

merged 19 commits into from
Feb 13, 2025

Conversation

MehrazRumman
Copy link
Contributor

PR #7814 Reverted .

@MehrazRumman
Copy link
Contributor Author

@auvipy can you review this PR ?

@codecov Codecov
Copy link

codecov bot commented Jan 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.24%. Comparing base (dbf2de8) to head (d4b1cb6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
celery/utils/term.py 50.00% 2 Missing ⚠️
celery/platforms.py 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9494      +/-   ##
==========================================
- Coverage   78.24%   78.24%   -0.01%     
==========================================
  Files         153      153              
  Lines       19041    19050       +9     
  Branches     2520     2520              
==========================================
+ Hits        14899    14905       +6     
- Misses       3856     3859       +3     
  Partials      286      286
Flag Coverage Δ
unittests 78.22% <76.92%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

auvipy
auvipy requested changes Jan 15, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

lets wait until our CI is stable

auvipy
auvipy requested changes Jan 15, 2025
View reviewed changes
celery/utils/term.py Outdated Show resolved Hide resolved
@MehrazRumman
Copy link
Contributor Author

lets wait until our CI is stable

Ok . Thanks .

@MehrazRumman MehrazRumman requested a review from auvipy January 17, 2025 15:00
@auvipy auvipy added this to the 5.5.0 milestone Feb 2, 2025
@auvipy
Copy link
Member

auvipy commented Feb 2, 2025

our CI is stable now, so lets see

@MehrazRumman
Copy link
Contributor Author

Thanks

auvipy
auvipy requested changes Feb 2, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you please fix the lint errors and improve test coverage as well? lint error fixing is a must

@Nusnus Nusnus marked this pull request as draft February 9, 2025 20:57
Nusnus
Nusnus requested changes Feb 9, 2025
View reviewed changes
Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

Changing to draft until the CI fully passes

@MehrazRumman MehrazRumman marked this pull request as ready for review February 12, 2025 19:09
@MehrazRumman MehrazRumman requested a review from auvipy February 12, 2025 19:10
@MehrazRumman
Copy link
Contributor Author

@auvipy I think you can review now !

Nusnus
Nusnus previously requested changes Feb 13, 2025
View reviewed changes
Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

Can you please rename the PR?
Also - this is just a pure git revert right? Or are there any manual modifications?

auvipy
auvipy requested changes Feb 13, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

@MehrazRumman can you improve the test coverage please?

celery/utils/term.py Show resolved Hide resolved
@MehrazRumman MehrazRumman requested a review from auvipy February 13, 2025 07:19
@MehrazRumman MehrazRumman changed the title PR #7814 reverted PR #7814 reverted & Add Test coverage for term.colored class Feb 13, 2025
@auvipy auvipy changed the title PR #7814 reverted & Add Test coverage for term.colored class Reverted PR #7814 & minor code improvement Feb 13, 2025
auvipy
auvipy approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

the code looks much better now. also thanks for the additional test cases. the smoke test failure seems to be unrelated to this PR. which is a timeout error

@auvipy auvipy dismissed Nusnus’s stale review February 13, 2025 07:59

review addressed

@auvipy auvipy merged commit bbe7c2e into celery:main Feb 13, 2025
68 of 70 checks passed
@auvipy auvipy mentioned this pull request Feb 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auvipy auvipy auvipy approved these changes

@Nusnus Nusnus Nusnus left review comments

Assignees
No one assigned
Labels
Issue Type: Regression PR Type: Bugfix
Projects
None yet
Milestone
5.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MehrazRumman @auvipy @Nusnus