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.

Sign up for GitHub

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

Restore missing __main__ logs #896

Merged
merged 4 commits into from May 21, 2022
Merged

Restore missing __main__ logs #896

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
698f940
Use twine logger to ensure main always logs
bhrutledge May 8, 2022
a49fe51
Revert "Use twine logger to ensure main always logs"
bhrutledge May 8, 2022
faa1bdb
Configure root logger instead of twine logger
bhrutledge May 8, 2022
aef83d5
Add changelog entry
bhrutledge May 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions twine/cli.py
View file
Open in desktop
Expand Up @@ -52,6 +52,7 @@ def configure_output() -> None:
# test_main.py due to capsys not being cleared.
logging.config.dictConfig(
{
"disable_existing_loggers": False,
"version": 1,
"handlers": {
"console": {
Expand All @@ -61,10 +62,8 @@ def configure_output() -> None:
"highlighter": rich.highlighter.NullHighlighter(),
}
},
"loggers": {
"twine": {
"handlers": ["console"],
},
"root": {
"handlers": ["console"],
Copy link
Contributor Author

@bhrutledge bhrutledge May 8, 2022 •
edited

Choose a reason for hiding this comment

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

@sigmavirus24 @jaraco I think this configuration means that any logs from dependencies will come through. By default, that would be WARNING and above; --verbose would enable INFO. I'm not sure if that's desired behavior or not; it could be useful, or it could be noisy/confusing.

Another option that maintains the current behavior of only enabling twine logs is to explicitly use the twine logger in __main__, which I started with, but reverted: 698f940

Copy link

Choose a reason for hiding this comment

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

Huh, that actually looks a bit nicer (hardcoding the logger's name to be "twine"). Whether you put that back or not, I've learned from this to always use a hardcoded logger name instead of __name__ if there's a chance of the file in question being executed as __main__...
This is one of those cases where Python's __name__ == '__main__' weirdness shows itself for the hack it is. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've learned from this to always use a hardcoded logger name instead of __name__ if there's a chance of the file in question being executed as __main__

@bayersglassey-zesty I think it's okay to use __name__ in that situation, under most circumstances. The reason it was an issue here is because I disabled all the other existing loggers when setting up the twine logger.

Copy link

Choose a reason for hiding this comment

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

That's fair.
My thinking is, I don't think there's ever a case where I want to target the logger for __main__.
Like, it would be weird to do logging.getLogger('__main__').setLevel(logging.INFO), because then if one of the functions in __main__ were ever factored out, it would suddenly be using a different logger.
You know?
Anyway. Weird silly edge cases. But something tells me it's nicer to know for sure what your logger's name is. 🤷

That said, I have absolutely no preference in this case, and actually I guess I've also learned that maybe it's nicer to run things by their "entry points" (as defined by pip and installed into virtualenv's bin directory), since then none of the library's modules are being loaded as __main__ and possibly causing __name__ weirdness...

Copy link

Choose a reason for hiding this comment

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

Actually, I just looked at which twine and clued into the fact that it's not even twine/__init__.py which ends up being run, it's twine/__main__.py.
And if I make my own Python module with __init__.py and __main__.py in it, it's the __main__.py which gets run when I run the module with python -m.
I learned something today...

And here are the docs for that: https://docs.python.org/3/library/__main__.html#main-py-in-python-packages

Copy link

Choose a reason for hiding this comment

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

...so actually, I guess what I'll be taking away from this is... to keep __main__.py as short as possible, something like:

from my_library import main
main()

...so that all the code -- in particular, any code which creates loggers or uses __name__ for anything other than the == '__main__' check -- lives in a separate module. 🤔

Edit: the docs actually say __main__.py shouldn't even bother with an if __name__ == '__main__' check, so I updated my code snippet! https://docs.python.org/3/library/__main__.html#id1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bayersglassey-zesty I'm glad you learned something from this. 😉

I guess what I'll be taking away from this is... to keep __main__.py as short as possible

That's a good point. Most of Twine's CLI logic is in cli.dispatch, but this PR shows that there's some non-trivial setup that could be moved to something like cli.main. I might follow-up with that refactoring.

maybe it's nicer to run things by their "entry points" (as defined by pip and installed into virtualenv's bin directory)

I think this depends on the tool and the situation. One nice thing about using python -m package is that you know that you're using the package that's installed in the python environment that you're using. When you run the package entrypoint, it'll be whichever one is found first on your $PATH, which might not be the same as python. This is particularly relevant for pip, and why it's a good habit to always use python -m pip.

Copy link
Member

Choose a reason for hiding this comment

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

@sigmavirus24 @jaraco I think this configuration means that any logs from dependencies will come through. By default, that would be WARNING and above; --verbose would enable INFO. I'm not sure if that's desired behavior or not; it could be useful, or it could be noisy/confusing.

Another option that maintains the current behavior of only enabling twine logs is to explicitly use the twine logger in __main__, which I started with, but reverted: 698f940

I suspect it would be undesirable to include output from dependencies if the context (logger name) isn't present. On the other hand, since twine is using logging as the mechanism for output, maybe that's what one would expect. I'm fine either way. Let's try it out and see how it behaves.

},
}
)
Expand Down