Skip to content

Updated logging.py for #9146 #9149

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

Merged
merged 10 commits into from
Jun 20, 2023
Merged

Updated logging.py for #9146 #9149

merged 10 commits into from
Jun 20, 2023

Conversation

Vijay-Arora
Copy link
Contributor

@Vijay-Arora Vijay-Arora commented Oct 1, 2021

Close #9146

Vijay-Arora and others added 6 commits October 1, 2021 19:34

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Updated logging.py for pytest-dev#9146

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Updated logging.py for pytest-dev#9146

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Create 9146.doc.rst

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Updated AUTHORS and added changelog file
@nicoddemus
Copy link
Member

Tip: you can add Closes #9146 to the PR description, which serves both to link the PR to the issue, and GitHub will close the issue automatically after the PR is merged (just added it myself). 👍

@Vijay-Arora
Copy link
Contributor Author

Thanks! But I see there are some checks which I see failed for eg. At least 1 approving review is required by reviewers with write access. What should I do in this case? Should I update the PR or let it be?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @Vijay-Arora! Left a few request for changes, please take a look.

cc @MetRonnie

@nicoddemus
Copy link
Member

At least 1 approving review is required by reviewers with write access. What should I do in this case? Should I update the PR or let it be?

No change required on your side, this just means at least one maintainer needs to approve the PR.

@Vijay-Arora
Copy link
Contributor Author

Vijay-Arora commented Oct 1, 2021

Thanks @Vijay-Arora! Left a few request for changes, please take a look.

cc @MetRonnie

Hey @nicoddemus, should I go ahead and make the requested changes and open a new PR for this? Also, I see some failures, do I need to make some changes for that too?

@nicoddemus
Copy link
Member

nicoddemus commented Oct 1, 2021

Hey @nicoddemus, should I go ahead and make the requested changes and open a new PR for this?

Just push new commits to the same branch and the PR will update automatically.

Also, I see some failures, do I need to make some changes for that too?

Hmm ignore them for now, they don't seem related to your PR, I will investigate those later.

@Vijay-Arora
Copy link
Contributor Author

Hey @nicoddemus, should I go ahead and make the requested changes and open a new PR for this?

Just push new commits to the same branch and the PR will update automatically.

Also, I see some failures, do I need to make some changes for that too?

Hmm ignore them for now, they don't seem related to your PR, I will investigate those later.

Cool, thanks, Making the changes now.

Vijay-Arora and others added 3 commits October 1, 2021 21:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! As the author of the original issue, I've added my thoughts

@MetRonnie
Copy link
Contributor

@nicoddemus I do wonder if the docstring for at_level should be updated at the same time, but I don't know whether the behaviour is also a threshold or not, I've never used it.

@pralkarz
Copy link
Contributor

@MetRonnie Good catch. As far as I understand it, the behavior is the same for at_level as it also utilizes the setLevel method, therefore an update for its docs seems reasonable.

@Vijay-Arora Would you be interested in driving this one home? 😄

@nicoddemus nicoddemus requested a review from twmr February 23, 2022 18:17

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Ronnie Dutta <[email protected]>
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Let's get this merged!

@Zac-HD Zac-HD dismissed nicoddemus’s stale review June 20, 2023 03:34

Requested changes were made.

@Zac-HD Zac-HD merged commit b5ec092 into pytest-dev:main Jun 20, 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.

Documentation for caplog set_level() should make it clearer it's a threshold
6 participants