-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
gh-138122: Refactor the CLI of profiling.sampling into subcommands #141813
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
Conversation
0790c5b to
1fcfffc
Compare
1fcfffc to
db2ed3b
Compare
|
@savannahostrowski I have reformulated the CLI to break it into sub commands. Could you play with this a bit and tell me what you think? |
|
CC @lkollar |
…nto subcommands
|
@hugovk if you have some time can you play with the CLI and tell be if you see any improvements we could do? |
savannahostrowski
left a comment
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 refactoring makes the UX so much less intimidating off the jump IMO. I really like what it does for help tbh. Makes things a bit more intuitive!
One suggestion around how we can clean up examples in the help but otherwise, LGTM!
lkollar
left a comment
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 played around with it a bit and the subcommands make the CLI much nicer.
One odd thing I noticed is that both run and attach is fine with --pstats and any of the pstats format options with --live. We should probably block these in live mode.
hugovk
left a comment
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.
We'll need to update the examples at https://docs.python.org/3.15/whatsnew/3.15.html#pep-799-high-frequency-statistical-sampling-profiler but this can also be a followup PR once this is settled.
Looks good, thanks!
| import collections | ||
| import marshal | ||
|
|
||
| from _colorize import ANSIColors |
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.
Rather than using ANSIColors directly, it would be good to set and use a theme instead, so people can easily customise. Can be a followup PR.
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.
Rather than using
ANSIColorsdirectly, it would be good to set and use a theme instead, so people can easily customise. Can be a followup PR.
Yeah good point, will do in a separate PR since the live TUI also may need that
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
|
|
will fix this bug |
Uh oh!
There was an error while loading. Please reload this page.