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

Jump to bottom

gh-95516: Add param types and clarify param descriptions of LogRecord (GH-95517) #95517

Merged
merged 2 commits into from Aug 2, 2022
Merged

gh-95516: Add param types and clarify param descriptions of LogRecord (GH-95517) #95517

merged 2 commits into from Aug 2, 2022

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Aug 1, 2022 •
edited by bedevere-bot

Adds missing parameter type information to the existing structured Sphinxdoc parameter list of logging.LogRecord, following the same format as PR #94701 did for logging.Formatter.

In addition, it clarifies several ambiguities in the existing parameter description text, particularly related to the types to pass. Most notably:

  • Use semantically-accurate and consistent reST/Sphinx syntax and roles throughout
  • Fix several bad references in the function description
  • Refine potentially confusing phrasing in the name parameter regarding where the name will retain its value
  • Fix and improve currently contradictory examples in level that implies string levels are used (when in fact it is numeric levels), and link the table where these levels are defined
  • Clarify exactly what is passed as a pathname (a full string path, not a Path or some bespoke "pathname" entity)
  • Fix comma splice in msg
  • Describe and link what an "exception tuple" is and where it comes from in exc_info

Before and after:

image

Fixes #95516

@CAM-Gerlach CAM-Gerlach requested a review from vsajip as a code owner Aug 1, 2022
@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Aug 1, 2022
vsajip
vsajip requested changes Aug 1, 2022
View changes
Copy link
Member

@vsajip vsajip left a comment

Thanks for the improvement, I've suggested one minor change.

Doc/library/logging.rst Outdated
where the logging call was made.
:type lineno: int

:param msg: The event description message;
Copy link
Member

@vsajip vsajip Aug 1, 2022

Choose a reason for hiding this comment

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

Perhaps change to

The event description message, which can be a %-format string
with placeholders for variable data.

Copy link
Member Author

@CAM-Gerlach CAM-Gerlach Aug 2, 2022

Choose a reason for hiding this comment

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

Sure, that's way better, thanks. I've made that an actual suggestion and applied it.

@bedevere-bot
Copy link

bedevere-bot commented Aug 1, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

CAM-Gerlach
CAM-Gerlach commented Aug 2, 2022
View changes
Doc/library/logging.rst Outdated Show resolved Hide resolved
@CAM-Gerlach CAM-Gerlach requested a review from vsajip Aug 2, 2022
@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Aug 2, 2022

Thanks for the great feedback @vsajip , and sorry for the delay—was busy taking care of other docs issues and PRs.

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Aug 2, 2022

Thanks for making the requested changes!

@vsajip: please review the changes made to this pull request.

@vsajip vsajip changed the title gh-95516: Add param types and clarify param descriptions of LogRecord gh-95516: Add param types and clarify param descriptions of LogRecord (GH-95517) Aug 2, 2022
@vsajip vsajip merged commit 75a6441 into python:main Aug 2, 2022
14 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Aug 2, 2022

Thanks @CAM-Gerlach for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

miss-islington commented Aug 2, 2022

Thanks @CAM-Gerlach for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Aug 2, 2022

GH-95565 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 2, 2022
@CAM-Gerlach @miss-islington
pythongh-95516: Add param types and clarify param descriptions of Log…
459842c 
…Record (pythonGH-95517)

(cherry picked from commit 75a6441)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@bedevere-bot
Copy link

bedevere-bot commented Aug 2, 2022

GH-95566 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 2, 2022
@CAM-Gerlach @miss-islington
pythongh-95516: Add param types and clarify param descriptions of Log…
c3392f6 
…Record (pythonGH-95517)

(cherry picked from commit 75a6441)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
vsajip pushed a commit that referenced this pull request Aug 2, 2022
@miss-islington @CAM-Gerlach
[3.11] gh-95516: Add param types and clarify param descriptions of Lo…
eff6778 
…gRecord (GH-95517) (GH-95566)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
erlend-aasland pushed a commit that referenced this pull request Aug 2, 2022
@miss-islington @CAM-Gerlach
gh-95516: Add param types and clarify param descriptions of LogRecord (…
a1579ad 
…GH-95517) (#95565)

(cherry picked from commit 75a6441)

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsajip vsajip

Assignees
No one assigned
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

logging.LogRecord missing param type information
4 participants
@CAM-Gerlach @bedevere-bot @miss-islington @vsajip