Skip to content

GH-88597: Added command line interface to UUID module. #99463

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

Merged
merged 3 commits into from
Jan 22, 2023

Conversation

achhina
Copy link
Contributor

@achhina achhina commented Nov 14, 2022

UUID module now supports command line usage.

❯ ./python.exe -m uuid             
5f2d57b1-90e8-417c-ba5d-69b9b6f74289

❯ ./python.exe -m uuid -h          
usage: uuid.py [-h] [-u {uuid1,uuid3,uuid4,uuid5}] [-ns NAMESPACE] [-n NAME]

Generates a uuid using the selected uuid function.

options:
  -h, --help            show this help message and exit
  -u {uuid1,uuid3,uuid4,uuid5}, --uuid {uuid1,uuid3,uuid4,uuid5}
                        The function to use to generate the uuid. By default uuid4 function is used.
  -ns NAMESPACE, --namespace NAMESPACE
                        The namespace used as part of generating the uuid. Only required for uuid3/uuid5 functions.
  -n NAME, --name NAME  The name used as part of generating the uuid. Only required for uuid3/uuid5 functions.
  • Tests ran locally

@achhina
Copy link
Contributor Author

achhina commented Nov 14, 2022

Hi @nanjekyejoannah, as you reviewed the first PR for this issue, would you be able to take a look at this one when you get a chance? I ended up moving the CLI logic to a separate function and importing & using argparse only in there to keep the rest of the module lightweight.

@achhina achhina force-pushed the GH-88597 branch 3 times, most recently from 81c6b06 to 99df7ef Compare November 14, 2022 09:23
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you!

I know that not all CLI entrypoints are tested, but I think we should start doing it. Especially because you have logic in your main, not just some other function call.

@achhina achhina force-pushed the GH-88597 branch 2 times, most recently from 31edacd to cc423c8 Compare November 15, 2022 02:50
@achhina achhina closed this Nov 15, 2022
@achhina achhina reopened this Nov 15, 2022
@achhina achhina force-pushed the GH-88597 branch 2 times, most recently from a0416f1 to e7fa43b Compare November 15, 2022 07:20
@achhina
Copy link
Contributor Author

achhina commented Nov 15, 2022

Hi @sobolevn, thanks for taking a look! I believe I've addressed all the comments, and have added in some unit tests. Would you be able to take a look again when you have time?

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! Several more ideas / comments :)

I think this can also be added to whatsnew/3.12, because this feature is quite useful.

@achhina
Copy link
Contributor Author

achhina commented Nov 16, 2022

Hi @sobolevn! I've tried to address all the comments you provided, would you be able to take another look?

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

LGTM! But, this will require a core-dev acceptance as well.

@gpshead
Copy link
Member

gpshead commented Jan 22, 2023

thanks!

@merwok
Copy link
Member

merwok commented Jan 22, 2023

For next PRs to Python repos, please don’t force push, it makes reviews less nice (comments can lose context, github notifs don’t link to a useful page, etc). See https://devguide.python.org/getting-started/pull-request-lifecycle/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants