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.

Sign up for GitHub

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

Jump to bottom

Reinstate CubeList._repr_html_() #4976

Merged
merged 6 commits into from
Sep 23, 2022
Merged

Reinstate CubeList._repr_html_() #4976

merged 6 commits into from
Sep 23, 2022

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Sep 21, 2022 •
edited
Loading

🚀 Pull Request

To address #4973

NOTE: aiming this at a bugfix release, so it is derived from, and targetting '3.3.x' branch.

TODO:

  • add whatsnew
    • ( create appropriate new whatsnew/latest.rst, on v3.3.x aiming for 3.3.1 )

@pp-mo
Copy link
Member Author

pp-mo commented Sep 21, 2022 •
edited
Loading

Pure tests to start with -- proving that the test for Cubelist._repr_html_() will fail.

@pp-mo
Copy link
Member Author

pp-mo commented Sep 21, 2022

... and that should fix it.

@pp-mo pp-mo changed the base branch from main to v3.3.x September 21, 2022 16:50
@pp-mo pp-mo marked this pull request as ready for review September 21, 2022 17:06
@pp-mo
Copy link
Member Author

pp-mo commented Sep 21, 2022

Ok, naming @trexfeathers and @lbdreyer as possible reviewers.
@lbdreyer simply because I'm still easing into Pytest usage.

We need to consider the implications for this going in a 3.3.1 release.
TODO: (for me) work out what a whatsnew "latest.rst" should look like in this case + initiate it.

@pp-mo pp-mo changed the title Test the calls implementing Cube and Cubelist printing, and repr_html(). Reinstate CubeList._repr_html_() Sep 21, 2022
@lbdreyer
Copy link
Member

Regarding

TODO: (for me) work out what a whatsnew "latest.rst" should look like in this case + initiate it.

Rather than editing latest.rst. I think you just need to edit the 3.3.rst to add a section for bug fixes (as we did in https://github.com/SciTools/iris/pull/4604/files) and then add your whats new entry to this?

@pp-mo
Copy link
Member Author

pp-mo commented Sep 22, 2022

Regarding

TODO: (for me) work out what a whatsnew "latest.rst" should look like in this case + initiate it.

Rather than editing latest.rst. I think you just need to edit the 3.3.rst to add a section for bug fixes (as we did in https://github.com/SciTools/iris/pull/4604/files) and then add your whats new entry to this?

Ok thanks, that makes sense !
I tried to consult the developer docs, but it isn't really clear about this IMHO

lbdreyer
lbdreyer requested changes Sep 22, 2022
View reviewed changes
Copy link
Member

@lbdreyer lbdreyer left a comment

Choose a reason for hiding this comment

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

Tests look great! Just a few small comments

lib/iris/tests/unit/cube/test_Cube.py Outdated
def test_two_with_same_name(self, cube):
# If a cube has two _DimensionalMetadata objects with the same name, the
def test_two_with_same_name(self, simplecube):
# If a simplecube has two _DimensionalMetadata objects with the same name, the
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to update these comments as "If a cube has two _DimensionalMetadata objects..." makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Rename+replace gone too far !

lib/iris/tests/unit/cube/test_Cube.py Outdated
class TestReprs:
"""
Confirm that str(cube), repr(cube) and cube.summary() work by creating a fresh
fresh :class:`iris._representation.cube_printout.CubePrinter` object, and using it
Copy link
Member

Choose a reason for hiding this comment

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

You have the word fresh twice here
"work by creating a fresh fresh :class:iris._representation.cube_printout.CubePrinter"

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 !

lib/iris/tests/unit/cube/test_CubeList.py Outdated

target = "iris.experimental.representation.CubeListRepresentation"
instance_mock = mock.MagicMock(
repr_html=mock.MagicMock() # return_value='') # NB this must return a string
Copy link
Member

Choose a reason for hiding this comment

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

Is this true as the return value doesn't seem to be being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 good spot !
Copied from the 'cube' case, where it did matter -- will fix.

@pp-mo
Copy link
Member Author

pp-mo commented Sep 22, 2022

Thanks @lbdreyer
Hope I have fixed those things now.

N.B. I am just about to check actual notebook output, but I tidied my conda + it is taking ages to build a test env ...
Best to wait for that.

@pp-mo pp-mo mentioned this pull request Sep 22, 2022
Merged
@lbdreyer lbdreyer mentioned this pull request Sep 23, 2022
Merged
@pp-mo
Copy link
Member Author

pp-mo commented Sep 23, 2022

Ok I think this will work now :
Fixed the glitch in the whatsnew.
I also checked that it does work as expected in a notebook ✔️

lbdreyer
lbdreyer reviewed Sep 23, 2022
View reviewed changes
docs/src/whatsnew/3.3.rst Outdated Show resolved Hide resolved
@lbdreyer lbdreyer merged commit 1d6d979 into SciTools:v3.3.x Sep 23, 2022
@lbdreyer
Copy link
Member

Whoops, Looks like there was a problem with the docs build from this merge: https://readthedocs.org/projects/scitools-iris/builds/18152663/

@pp-mo
Copy link
Member Author

pp-mo commented Sep 26, 2022

Whoops, Looks like there was a problem with the docs build from this merge: https://readthedocs.org/projects/scitools-iris/builds/18152663/

Some kind of HDF error, could be a bit of a random glitch ?
Just trying a re-build : https://readthedocs.org/projects/scitools-iris/builds/18168111/

@pp-mo
Copy link
Member Author

pp-mo commented Sep 26, 2022

Ok it has built, but my new whatsnew is not properly contained in the dropdown section
Need more indent.
However, I think we should shortly be adding to this section in #4933, so I will fix it there ...

@pp-mo pp-mo deleted the fix_cubelist_print branch September 26, 2022 13:54
stephenworsley added a commit to stephenworsley/iris that referenced this pull request Sep 29, 2022
@stephenworsley
Merge branch 'v3.3.x' into mergeback_v3.3.1
c4781ec 
* v3.3.x:
  Update whatsnew for 3.3.1 release (SciTools#5002)
  Port dependency fixes to `v3.3.x`. (SciTools#4992)
  Fix name loader problem (SciTools#4933)
  Reinstate CubeList._repr_html_() (SciTools#4976)

# Conflicts:
#	.github/workflows/ci-tests.yml
#	requirements/ci/nox.lock/py310-linux-64.lock
#	requirements/ci/nox.lock/py38-linux-64.lock
#	requirements/ci/nox.lock/py39-linux-64.lock
#	requirements/ci/py310.yml
#	requirements/ci/py38.yml
#	requirements/ci/py39.yml
#	setup.cfg
@rcomer rcomer mentioned this pull request Oct 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbdreyer lbdreyer lbdreyer approved these changes

@trexfeathers trexfeathers Awaiting requested review from trexfeathers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pp-mo @lbdreyer