-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add some missing logging features #5388
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
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
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, that's very thorough! A few minor notes below about the things you mentioned.
def usesTime(self) -> bool: ... # undocumented | ||
|
||
class BufferingFormatter: | ||
linefmt: Formatter |
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 think line format could be a string like: '%(something)s %(something)s'. So, linefmt
should be changed to:
linefmt: Union[str, Formatter]
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.
Technically it can be anything with a .format()
method, even a string. But that .format()
method will be called with only one argument, a log record object. It is possible to create a string that formats them in a useful way, such as "{0.name}: {0.msg}"
, although that doesn't seem to be how it's intended to be used.
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.
Yup!
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.
So the correct fix would be a protocol? We could leave as Any
for the moment, though.
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.
Honestly I would just use Formatter
. It's what the author of the code intended. (This is old code, likely written before str.format
was a thing)
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 was under the same impression as @Akuli, intended to be a Formatter
|
||
class BufferingFormatter: | ||
linefmt: Formatter | ||
def __init__(self, linefmt: Optional[Formatter] = ...) -> None: ... |
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.
Same here:
def __init__(self, linefmt: Optional[Union[str, Formatter]] = ...) -> None: ...
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.
If someone want to change Formatter
to a protocol, they can do it in a separate PR. I'm fine with it being a concrete class for now.
This handles #5257, #5015, and changes two
type
toType[...]
for #343Note I annotatedvalidation_pattern
,IDENTIFIER
, andextMatch
asAny
since I cannot do this, which would make a variable, not a type alias.Please note that
SMTPHandler.password
is not always created depending on what is passed in! But since I know of no workarounds for maybe-exsiting-attributes and the documents suggest:I have added it
password: str
.I am not fixing everything I see in this PR, I have left a lot of
logging.config
as-is