Skip to content

bpo-35970: Add help flag to base64 util #11789

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
Closed
wants to merge 0 commits into from
Closed

bpo-35970: Add help flag to base64 util #11789

wants to merge 0 commits into from

Conversation

rkuska
Copy link
Contributor

@rkuska rkuska commented Feb 8, 2019 •
edited by bedevere-bot
Loading

Probably a really unnecessary contribution but I failed today to print help message for base64 utility without an error:

 $ python3 -m base64 -help
option -h not recognized
usage: /usr/local/Cellar/python/3.7.2_1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/base64.py [-d|-e|-u|-t] [file|-]
        -d, -u: decode
        -e: encode (default)
        -t: encode and decode string 'Aladdin:open sesame

So I felt like this is a ripple in time space continuum worth adjusting. I understand that this part of code is probably a long forgotten and therefore I will not be surprised if you decide to just close this PR.

https://bugs.python.org/issue35970

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@remilapeyre
Copy link
Contributor

Hi @rkuska, thanks for your contribution!

This is bug and all bugs are worth fixing!

There is some process to follow before submitting changes to Python, you can read about them at https://devguide.python.org/pullrequest/#quick-guide

You first need to create an account on https://bugs.python.org/ and open a new issue.

Your Pull Request will also need a test before being accepted.

You should also use https://blurb-it.herokuapp.com/ to add a news entry to your PR.

@rkuska rkuska changed the title base64: Add flag for help message bpo-35970: Add help flag to base64 util Feb 11, 2019
@rkuska
Copy link
Contributor Author

rkuska commented Feb 11, 2019

I've created an issue and changed commit message and also edited PR title and lastly news entry was added. I will add also tests once I will get some ack about my fix - not sure if getopt shouldn't be replaced by argparse.

remilapeyre
remilapeyre reviewed Feb 12, 2019
View reviewed changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

I'm not sure getopt needs to be replaced, optparse has been deprecated but not getopt.

Lib/base64.py Outdated
@@ -558,22 +558,25 @@ def decodestring(s):
def main():
"""Small main program"""
import sys, getopt
usage = """usage: %s [-h|-d|-e|-u|-t] [file|-]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can convert this to an f-string

remilapeyre
remilapeyre suggested changes Mar 19, 2019
View reviewed changes
Misc/NEWS.d/next/Library/2019-02-11-19-06-10.bpo-35970.ZRvh51.rst Outdated
@@ -0,0 +1 @@
Add help flag to base64 cmd utility
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Patch contributed by ...

@csabella csabella requested a review from serhiy-storchaka June 5, 2019 15:05
taleinat
taleinat requested changes Jun 8, 2019
View reviewed changes
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

This certainly would be a nice improvement.

Please add two simple tests checking that the usage text is printed:

  1. Running with -h
  2. Running with an invalid parameter

(See TestMain in test_base64.py)

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@csabella
Copy link
Contributor

@rkuska, please address the code review for @taleinat. Thank you!

@taleinat
Copy link
Contributor

Ping, @rkuska?

@ammaraskar ammaraskar closed this Oct 6, 2021
@ammaraskar
Copy link
Member

Oops, I think I accidentally closed this while trying to update rkuska's branch. Let me update this work in a new PR.

ammaraskar added a commit to ammaraskar/cpython that referenced this pull request Oct 6, 2021
@rkuska @ammaraskar
bpo-35970: Add help flag to base64 util (pythonGH-11789)
1eec62a 
Co-authored-by: Ammar Askar <ammar@ammaraskar.com>
@ammaraskar ammaraskar mentioned this pull request Oct 6, 2021
Merged
@ammaraskar
Copy link
Member

@taleinat would you mind giving #28774, a continuation of this PR a quick review?

miss-islington pushed a commit that referenced this pull request Oct 7, 2021
@ammaraskar
bpo-35970: Add help flag to base64 module (GH-28774)
5baec4a 
This continues off rkuska's work from #11789 seeing as the patch wasn't updated to add tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taleinat taleinat taleinat requested changes

@remilapeyre remilapeyre remilapeyre requested changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees
No one assigned
Labels
awaiting changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rkuska @the-knights-who-say-ni @remilapeyre @bedevere-bot @csabella @taleinat @ammaraskar