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

Refactored some logging statements to use lazy % formatting #3326

Merged
merged 7 commits into from
Mar 7, 2023

Conversation

WhiteWolf47
Copy link
Contributor

@WhiteWolf47 WhiteWolf47 commented Mar 6, 2023

Refactored some logging statements in bugbug/scripts/regressor_finder.py
to use % lazy format.

This PR is part of the fix for #3323 .
This goal of this PR is to refactor some of the logging statements in bugbug/scripts/regressor_finder.py to use the % lazy style of formatting
for example : refactoring this statement -> logger.info("Analyzing {}...".format(bug_fixing_commit["rev"]))
to logger.info("Analyzing %s...", bug_fixing_commit["rev"])

@WhiteWolf47
Copy link
Contributor Author

@suhaibmujahid can you please review this? 😊

Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

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

The goal of #3323 is to refactor all cases. However, you do not need to refactor all of them yourself. Will close #3323 when there are no more cases left.

BTW. This pull request is not linked to an issue. See here how to link a pull request to an issue: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@suhaibmujahid
Copy link
Member

@WhiteWolf47 I think resolving #3323 is not your goal. Could you please update the description to mention that this pull request is part of the fix for #3323? Also, could you reflect the goal of the pull request in the description? The description mentions regressor_finder.py where the patch is changing regressor_finder.py as well.

@WhiteWolf47
Copy link
Contributor Author

@WhiteWolf47 I think resolving #3323 is not your goal. Could you please update the description to mention that this pull request is part of the fix for #3323? Also, could you reflect the goal of the pull request in the description? The description mentions regressor_finder.py where the patch is changing regressor_finder.py as well.

Will do that!

@WhiteWolf47
Copy link
Contributor Author

@suhaibmujahid what you say now :)

Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

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

Could you please discard unrelated cahnges (the deleted lines at the end of .gitignore and scripts/regressor_finder.py)?

bugbug/models/duplicate.py Outdated Show resolved Hide resolved
@suhaibmujahid
Copy link
Member

Also, please fix the lint errors.

To avoid lint failures, it is recommended to setup pre-commit: https://github.com/mozilla/bugbug#auto-formatting

@suhaibmujahid suhaibmujahid linked an issue Mar 7, 2023 that may be closed by this pull request
@WhiteWolf47
Copy link
Contributor Author

WhiteWolf47 commented Mar 7, 2023

@suhaibmujahid i've setup the pre-commit , still the lint error is there 🤔 . Also the pre-commit tests are passing perfectly normal in my local repo

@suhaibmujahid
Copy link
Member

@suhaibmujahid i've setup the pre-commit , still the lint error is there 🤔 . Also the pre-commit tests are passing perfectly normal in my local repo

pre-commit runs on the new changes only. If the change is already committed, it will ignore it, but the CI will fail. To run pre-commit on everything, please run pre-commit run --all-files.

@WhiteWolf47
Copy link
Contributor Author

@suhaibmujahid i've setup the pre-commit , still the lint error is there thinking . Also the pre-commit tests are passing perfectly normal in my local repo

pre-commit runs on the new changes only. If the change is already committed, it will ignore it, but the CI will fail. To run pre-commit on everything, please run pre-commit run --all-files.

okay lemme try this!

@WhiteWolf47
Copy link
Contributor Author

WhiteWolf47 commented Mar 7, 2023

@suhaibmujahid "hook id : black" this one has failed, everything else has passed. But running "pre-commit run --all-files" again passes everything.

@WhiteWolf47
Copy link
Contributor Author

@suhaibmujahid can you review this now, all checks have passed 😊

suhaibmujahid
suhaibmujahid previously approved these changes Mar 7, 2023
Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you, @WhiteWolf47!

scripts/regressor_finder.py Outdated Show resolved Hide resolved
@WhiteWolf47 WhiteWolf47 changed the title Refactored some logging statemnts to use lazy % formatting Refactored some logging statements to use lazy % formatting Mar 7, 2023
@WhiteWolf47
Copy link
Contributor Author

LGTM. Thank you, @WhiteWolf47!

No problem man. Thanks for helping me out with the issue. Looking forward to more contributions to bugbug 😃

@WhiteWolf47
Copy link
Contributor Author

@suhaibmujahid did the recommended changes, can you please review and merge now 😊

Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

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

Thank you!

@suhaibmujahid suhaibmujahid merged commit 21a77f4 into mozilla:master Mar 7, 2023
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.

Refactor logging statements to use lazy % formatting
2 participants