-
Notifications
You must be signed in to change notification settings - Fork 1.3k
logger: output footer once for multithreaded push/pull errors #2674
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
Conversation
efiop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @n3hrox ! Thanks a lot for the PR! Please comment down below.
| # so won't be reused by any other subsequent run anyway. | ||
| clean_repos() | ||
|
|
||
| if ret != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it should be here or rather in finally block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This place is fine.
| # so won't be reused by any other subsequent run anyway. | ||
| clean_repos() | ||
|
|
||
| if ret != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This place is fine.
| nc=colorama.Fore.RESET, | ||
| levelname=record.levelname, | ||
| description=self._description(record.msg, exception), | ||
| msg=record.msg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that deepsource detected that msg is not used here although I did not delete it
I think adding it back to message or removing it entirely should be scope of another PR to be honest, as it affects multiple test cases.
What do you think @efiop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is out of scope for this PR.
efiop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🎉
Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? If yes, have you created a PR for
dvc.org documenting it or at
least opened an issue for it? If so, please add a link to it.
This should fix #2553
There is one test already for
_downloadso it should be sufficient.If you have any suggestions how could I test the footer appearance deduplication with pytest please let me know! This is my first PR for open source repo so any comments are welcome.