-
Notifications
You must be signed in to change notification settings - Fork 275
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
updated docs and comments #1475
updated docs and comments #1475
Conversation
Updated/removed documented commands and comments which were referencing Python2. Also updated links to documentation referencing Python2 docs (unchanged where needed) Signed-off-by: Samuel Gregorovic <[email protected]> Signed-off-by: samuelgregorovic <[email protected]>
56ebe34
to
7c8774b
Compare
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.
LGTM, thanks!
np :) |
Thanks for taking the time to work on this @samuelgregorovic! I don't think we need to change tox.ini and the GitHub workflows, they are running in environments where python is a link to python3 already. |
@joshuagl i just thought it would be better for consistency as mentioned in the issue, even though in some places it was not necessary. thank you for the heads up anyway! |
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.
LGTM, left one nitpick but it's fine as is...
@joshuagl did you have real doubts about tox and the workflows or was it just a comment?
# FIXME: We need a way to tell Python 2, but not Python 3, to return | ||
# filenames in Unicode; see #61 and: | ||
# http://docs.python.org/2/howto/unicode.html#unicode-filenames | ||
# http://docs.python.org/howto/unicode.html#unicode-filenames |
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 believe the whole comment here could be removed
There is a small conflict now though, would you mind resolving that? |
@jku done |
No real doubts, just cautious about changing things that don't need it. I think the consistency argument @samuelgregorovic made is a reasonable one. Thanks for checking @jku and thanks for the patch @samuelgregorovic ! |
So I intended to merge this. Then I noticed I had asked to "resolve" the conflict without suggesting rebase. Github UI makes it so easy to merge instead so samuel had done that ... and this creates a git history that is too complex for my small brain. So I figured I'd rebase it myself, make a PR to get CI results (just in case), but now of course I can't merge that one because I can't review my own PR 🤦 Soo, @joshuagl maybe easiest if you approve #1493 (content is identical to this one, just rebased instead of reverse-merged)? |
Merged in #1439, thanks @samuelgregorovic |
@jku np |
Fixes #1349
updated docs and comments (exclude Python2 end-of-life):
Updated/removed documented commands and comments which were referencing Python2. Also updated links to documentation referencing Python2 docs (unchanged where needed)
Signed-off-by: Samuel Gregorovic [email protected]