GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||
GeezerVison is showing github.com — GH-88597: Added command line interface to UUID module. by achhina · Pull Request #99463 · python/cpython Conversation
|
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
81c6b06 to
99df7ef
Compare
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
31edacd to
cc423c8
Compare
a0416f1 to
e7fa43b
Compare
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Hi @sobolevn! I've tried to address all the comments you provided, would you be able to take another look? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
thanks! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.