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

Add json option to lsmagic #14331

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

s-kai273
Copy link

Summary

This PR is based on the discussion in issue#11793.
When executing lsmagic in a Lab environment, the results are returned in JSON format, and following that, I have made it possible to obtain similar ones by adding a --json option.

Change

  • Add json option to lsmagic
    implementation code reffers to this comment

md = MagicsDisplay(self.shell.magics_manager, ignore=[])
if args.json:
return md._repr_json_()
return md
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a simple either/or, I would suggest the slightly more immediately explicit

if args.json:
    return md._repr_json_()
else:
    return md

But otherwise looks good to me.

Thanks!!

Copy link
Author

Choose a reason for hiding this comment

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

@fperez
Thanks, for your review.
I modified at 36a3759

@fperez
Copy link
Member

fperez commented Mar 12, 2024

Pinging @ccordoba12 for a heads-up on the QtConsole side if this gets merged, so you can sync.

Let's wait for his approval before merging - this change is fairly innocuous, but also not urgent and I don't want to force the QtConsole team to do a release due to a compatibility break.

@fperez
Copy link
Member

fperez commented Mar 17, 2024

With this change, all looks good to go for me, thanks @s-kai273!

@ccordoba12 - I don't want to merge this without your explicit OK so we don't accidentally trigger a problem for you and the QtConsole team, so please let us know if it's OK to merge or you need to first sync on anything else.

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