Skip to content
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

Share some options between the CLI and magic. #350

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Nov 20, 2024

This refactor compute_render_options, to be shared between CLI and magic.

Add a few of the CLI options to be available in the magic.

The only thing I don't really like is that we redefine the help text options.

@Carreau
Copy link
Contributor Author

Carreau commented Nov 20, 2024

See discussion in #334

@Carreau Carreau force-pushed the share-magic branch 2 times, most recently from c74cccf to edae59c Compare November 20, 2024 13:05
@Carreau Carreau closed this Dec 2, 2024
@Carreau Carreau reopened this Dec 2, 2024
@Carreau
Copy link
Contributor Author

Carreau commented Dec 2, 2024

Closing and reopening to trigger CI.

Copy link
Owner

@joerick joerick left a comment

Choose a reason for hiding this comment

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

sorry for the delay in the review. it looks good! just a couple comments

pyinstrument/__main__.py Outdated Show resolved Hide resolved
pyinstrument/__main__.py Outdated Show resolved Hide resolved
@Carreau
Copy link
Contributor Author

Carreau commented Jan 6, 2025

sorry for the delay in the review. it looks good

No needs to apologize, this is open source and users are not owed anything.
I've learn myself to instead of apologizing, thanking contributor for their patience – at least I try. It's a subtle difference, but at least help me with framing my guilt and decrease burnout.

I will look back into this when possible,

Happy new year as well.

Copy link
Owner

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Happy new year! And thanks for your patience. Very wise words, I'll try to take them on ;)

I think a small simplification and this is ready to go. If it's not clear I'm happy to jump in and make the change, just let me know.

pyinstrument/__main__.py Outdated Show resolved Hide resolved
This refactor compute_render_options, to be shared between CLI and
magic, it changes the last parameter from a file-like output to a
boolean to control wether or not the output will support color and
unicode.

The only thing I don't really like is that we redefine
the help text options.

Co-authored-by: Joe Rickerby <[email protected]>
@Carreau
Copy link
Contributor Author

Carreau commented Jan 23, 2025

And thanks for your patience

😄, thanks for yours.

Ah, the unicode/color support bit makes sense. Let's change the compute_render_options to use unicode_support and color_support (moving the file-related stuff to the call site) and then remove the _compute_render_options private version.

I'd rather do the idiomatic thing and raise exceptions on errors. I don't see a problem with magic.py importing OptionsParseError, it's already importing from main anyway. But if it is an issue, we could make it a ValueError and catch that instead.

It should be done, I now just let OptionsParseError bubble up, that seem to work if it's ok with you, so I don't even have to import it.

@joerick
Copy link
Owner

joerick commented Jan 23, 2025

Great! Looks good.

@joerick joerick merged commit 6eecceb into joerick:main Jan 23, 2025
21 checks passed
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.

2 participants