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

Convert print statements to logging statements #1893

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Convert print statements to logging statements #1893

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be22037
Convert print statements to logging statements
invisiblethreat Feb 29, 2024
cd7fa2e
Use caplog fixture
invisiblethreat Mar 1, 2024
44d9c83
Ran pre-commit hooks
invisiblethreat Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/snowflake/connector/auth/webbrowser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,17 +166,17 @@ def prepare(
)
return

print(
logger.info(
"Initiating login request with your identity provider. A "
"browser window should have opened for you to complete the "
"login. If you can't see it, check existing browser windows, "
"or your OS settings. Press CTRL+C to abort and try again..."
)

logger.debug("step 2: open a browser")
print(f"Going to open: {sso_url} to authenticate...")
Copy link
Contributor

Choose a reason for hiding this comment

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

We print it to stdout so when the program failed to open a browser, CLI users could manually open the link in browser, not sure if logging would work in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @sfc-gh-mkeller

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is at least a breaking change and should go through announcement. But I'm not too sure whether we'd want to change this in the first place. I think the opening message we wouldn't want to get rid off in the default case.
How about introducing a setting that switches to logging instead of printing, but we'd leave the default setting on printing? @invisiblethreat would that work for you?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that would work. I'd actually propose that log is the default and a more verbose mode is available for troubleshooting, or this particular use case. Defaulting a library to STDOUT feels like the wrong thing to do, but I'm not privy to all the use cases 😄

logger.info(f"Going to open: {sso_url} to authenticate...")
if not self._webbrowser.open_new(sso_url):
print(
logger.warning(
"We were unable to open a browser window for you, "
"please open the url above manually then paste the "
"URL you are redirected to into the terminal."
Expand Down
Loading