-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Quiet warnings about netrc #3267
Conversation
aiohttp/helpers.py
Outdated
netrc_path = home_dir.joinpath('.netrc') | ||
except RuntimeError as e: # pragma: no cover | ||
# if pathlib can't resolve home, it may raise a RuntimeError | ||
client_logger.warning("Could not find .netrc file: %s", e) |
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.
This message seems to be misleading
aiohttp/helpers.py
Outdated
""" handle error raised by pathlib """ | ||
client_logger.warning("could't find .netrc file: %s", e) | ||
return netrc_obj | ||
if os.name == 'nt': # pragma: no cover |
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.
Use platform.system()
instead
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.
netrc_path = home_dir / ('_netrc' if platform.system() == 'Windows' else '.netrc')
If there isn't a .netrc file specified by an environment variable, it can be confusing to see warnings about it. If NETRC isn't set, don't warn. Only warn if: a) can't resolve HOME, b) can load, but can't parse file, c) can't find file, d) file appears to exist at the default location but is unreadable for some reason.
I also couldn't understand why these warnings were printed in the first place. |
I dunno, if I just submit a diff where I delete all of them, that fine? |
Thanks! |
@nicktimko Why is it even necessary to print a warning if the home directory cannot be resolved? A typical Docker container will not have a resolvable home directory unless one is explicitly created in the Dockerfile. |
If your code is not supposed to work with |
@agronholm This was a bit monkey-see, monkey-do. I was trying to preserve the existing behavior that I didn't understand, as I haven't used a |
What are you talking about? I use |
Ok, I see nothing bad if we will suppress a warning if the home directory doesn't exist.. @agronholm would you prepare a pull request? |
Sure, I'll try to get that done this week. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs. |
What do these changes do?
If there isn't a .netrc file specified by an environment variable, it can be confusing to see warnings about it. If NETRC isn't set, don't warn. Only warn if one of
Are there changes in behavior for the user?
When using the client from a command line application, the user will no longer see
could't find .netrc file
if~/.netrc
is missing and/or the envvarNETRC
isn't set.Related issue number
None.
Checklist
CONTRIBUTORS.txt
: n/a trivial changeCHANGES
folder: trivial change?<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.