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

Use _repr_json_ rather than _repr_html_ #1130

Closed
wants to merge 2 commits into from

Conversation

jsignell
Copy link
Member

@jsignell jsignell commented May 23, 2023

Related Issue(s):

Description:

This PR is pretty extreme, but basically it rips out all the jinja templates and _repr_html_ methods and instead uses the ipython json display mechanism. To me this feels more like what I want to see as the display for a STAC object 🤷

Screenshot from 2023-05-23 10-36-49

Pros:

  • Cuts a lot of code
  • Size of saved notebooks is smaller
  • No extra GETs (_repr_html_ causes GETs #1097)
  • Potentially closer to what the user is used to for STAC objects?

Cons:

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

@pjhartzell
Copy link
Collaborator

I really like this. Complete information, familiar format, less code.

@gadomski gadomski added this to the 1.8 milestone May 25, 2023
@gadomski gadomski self-requested a review May 25, 2023 15:20
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

I'm a little notebook-slow, how do I get the nicely-formated JSON when I run (e.g.) the quickstart notebook locally? I just tried things out and it looks like it's just printing the dict:

Screenshot 2023-05-25 at 9 26 02 AM

Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to add a _repr_json_ for an item collection?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was getting it via inheritance

@jsignell
Copy link
Member Author

I'm a little notebook-slow, how do I get the nicely-formated JSON when I run (e.g.) the quickstart notebook locally? I just tried things out and it looks like it's just printing the dict:
Screenshot 2023-05-25 at 9 26 02 AM

Assuming you are in jupyter lab, you don't want the to_dict(). Just type asset in a cell and execute it

@gadomski
Copy link
Member

Hm, I'm just in the VSCode jupyter interface, and it looks like that doesn't do the pretty printing either. Is there some sort of extension I need to enable?

Screenshot 2023-05-25 at 10 55 17 AM

I think I can state a design goal: a vanilla install of our doc dependencies has pretty-printing in a local notebook, e.g. in VSCode.

@jsignell
Copy link
Member Author

Oh that is such a bummer. It looks like vscode hasn't implemented IPython.display.JSON 😢

image

@gadomski
Copy link
Member

Bummer indeed. I like the format you showed originally much better than the html. I'm a bit reluctant to just rip the HTML out, since so many folks use VSCode (and there may be other common environments where HTML is supported but JSON is not). I guess we could keep the html and just add json?

@jsignell
Copy link
Member Author

Yeah... I guess we could roll our own html that looks like the output of IPython.display.JSON and fall back to that on vscode. It would just take a bit longer to implement so I'd probably need someone to actually say they want that :)

@jsignell
Copy link
Member Author

jsignell commented Jun 1, 2023

Closing this for now since there is no built-in vscode support and it would take a non-trivial amount of work to implement a pretty json jinja template.

@jsignell jsignell closed this Jun 1, 2023
@jsignell jsignell deleted the no-repr-html branch June 1, 2023 19:29
This was unlinked from issues Jun 1, 2023
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.

3 participants