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

Migrate formatting_html.py into xarray core #10

Closed
wants to merge 16 commits into from

Conversation

eni-awowale
Copy link
Collaborator

Draft:

This PR migrates the formatting_html.py module to xarray/core/formatting_html.py as part of the on-going effort to merge xarray-datatree into xarray.

One thing of note is that importing and setting the OPTIONS to "default" in datatree/formatting_html.py (lines) were moved into xarray/core/options.py on #L23 and #L49. So, I did not add them back to xarray/core/formatting_html.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to delete formatting_html.py and test_formatting_html.py in datatree_.

@@ -18,6 +18,9 @@
from xarray.core.coordinates import DatasetCoordinates
from xarray.core.dataarray import DataArray
from xarray.core.dataset import Dataset, DataVariables
from xarray.core.formatting_html import (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated import statements

Copy link
Owner

@flamingbear flamingbear left a comment

Choose a reason for hiding this comment

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

I don't have strong opinions, this looks reasonable to me. If this were my PR I'd change some of the names since the new functions are merged into existing files and I'd want to make sure people knew the fxns were for datatree

@@ -341,3 +343,128 @@ def dataset_repr(ds) -> str:
]

return _obj_repr(ds, header_components, sections)


def summarize_children(children: Mapping[str, Any]) -> str:
Copy link
Owner

Choose a reason for hiding this comment

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

I might find a way to identify these moved files as datatree specific.
e.g summarize_datatree_children and datatree_node_repr, _wrap_datatree_repr. pick what makes sense to you and use that through out.

return request.param


class Test_summarize_datatree_children:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed these test classes so they would be more descriptive.

@eni-awowale eni-awowale force-pushed the DAS-2066-migrate-formatting-html branch from 3aba6da to e16a01c Compare April 11, 2024 16:07
Copy link

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Thankyou for working on this @eni-awowale ! The main question I have before diving deeper is whether or not you have any time to look into the annoying bug which squeezes the width of the html output - see xarray-contrib/datatree#91

@flamingbear
Copy link
Owner

This was merged to the main repo pydata#8930

@flamingbear flamingbear deleted the DAS-2066-migrate-formatting-html branch May 21, 2024 16:29
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.

5 participants