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

Removal of deprecations for Contour #26907

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

DHClimber
Copy link
Contributor

@DHClimber DHClimber commented Sep 24, 2023

PR summary

This change is related to [MNT]: Remove 3.7-deprecated API
#26865

This pull request removes deprecated classes and methods from contour.py and associated contour.pyi

"pre-commit.ci autofix"

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

The code that has been removed is not needed in the removal note, please refer to the deprecation note, which can be copy/pasted and updated to reflect the removal rather than deprecation:

``contour.ClabelText`` and ``ContourLabeler.set_label_props``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... are deprecated.
Use ``Text(..., transform_rotates_text=True)`` as a replacement for
``contour.ClabelText(...)`` and ``text.set(text=text, color=color,
fontproperties=labeler.labelFontProps, clip_box=labeler.axes.bbox)`` as a
replacement for the ``ContourLabeler.set_label_props(label, text, color)``.
``ContourLabeler`` attributes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The ``labelFontProps``, ``labelFontSizeList``, and ``labelTextsList``
attributes of `.ContourLabeler` have been deprecated. Use the ``labelTexts``
attribute and the font properties of the corresponding text objects instead.

Additionally, please rename the file with the PR number rather than 00001.

@ksunden ksunden changed the title deprecated Removal of deprecations for Contour Sep 25, 2023
@ksunden ksunden mentioned this pull request Sep 25, 2023
14 tasks
Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

Do you feel comfortable squashing the commits to avoid adding and removing the changelog file?

Other than that, just a bit of clean up of the changelog file left

Comment on lines 1 to 5
Removal change template
~~~~~~~~~~~~~~~~~~~~~~~

Enter description of methods/classes removed here....

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Removal change template
~~~~~~~~~~~~~~~~~~~~~~~
Enter description of methods/classes removed here....

Comment on lines 18 to 19
Please rename file with PR number and your initials i.e. "99999-ABC.rst"
and ``git add`` the new file.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Please rename file with PR number and your initials i.e. "99999-ABC.rst"
and ``git add`` the new file.

This is missing the body of that change note

@DHClimber
Copy link
Contributor Author

Do you feel comfortable squashing the commits to avoid adding and removing the changelog file?

Other than that, just a bit of clean up of the changelog file left

Thanks for the feedback, I'll try to figure it out. This is my first time and I'm not familiar with squashing, but I'll try.

@ksunden
Copy link
Member

ksunden commented Sep 26, 2023

We have some docs on squashing via git rebase -i:

https://matplotlib.org/stable/devel/development_workflow.html#rewrite-commit-history

@ksunden
Copy link
Member

ksunden commented Sep 26, 2023

Also remove from the corresponding .pyi file, please

@DHClimber DHClimber force-pushed the depreciated branch 2 times, most recently from 0e2d856 to 9c61e6c Compare September 26, 2023 04:50
@DHClimber
Copy link
Contributor Author

We have some docs on squashing via git rebase -i:

https://matplotlib.org/stable/devel/development_workflow.html#rewrite-commit-history

Thanks, I think I figured it out.

@DHClimber
Copy link
Contributor Author

It shows one test failing Mypy. I'm having trouble understanding what the issue is, any suggestions on what I need to do to correct?

@rcomer
Copy link
Member

rcomer commented Sep 26, 2023

I agree the Mypy message could be clearer, but I think it’s just that you have removed some methods from the .py file that you have not yet removed from the .pyi file.

Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

Just some lint things, if you can sqaush again, that might be ideal as well, though we can also just do that on merge

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The ``labelFontProps``, ``labelFontSizeList``, and ``labelTextsList``
attributes of `.ContourLabeler` have been removed. Use the ``labelTexts``
attribute and the font properties of the corresponding text objects instead.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
attribute and the font properties of the corresponding text objects instead.
attribute and the font properties of the corresponding text objects instead.

@@ -1909,4 +1871,4 @@ def _initialize_x_y(self, z):
<https://en.wikipedia.org/wiki/Marching_squares>`_ algorithm to
compute contour locations. More information can be found in
`ContourPy documentation <https://contourpy.readthedocs.io>`_.
""" % _docstring.interpd.params)
""" % _docstring.interpd.params)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
""" % _docstring.interpd.params)
""" % _docstring.interpd.params)

ksunden
ksunden previously approved these changes Sep 27, 2023
@QuLogic
Copy link
Member

QuLogic commented Sep 27, 2023

stubtest is still complaining; the entries corresponding to the removed ones need to be removed from the .pyi file as well. You can see its comments on the Files changed tab.

@ksunden ksunden dismissed their stale review September 27, 2023 05:12

stubtest failures still

@DHClimber DHClimber force-pushed the depreciated branch 2 times, most recently from de41f99 to dcfc245 Compare September 28, 2023 15:47
@DHClimber
Copy link
Contributor Author

I'm completely lost at this point. I don't understand where all the problems are coming from.

@ksunden
Copy link
Member

ksunden commented Sep 28, 2023

You seem to have done the (roughly) same thing you did previously regarding rebasing.

If you only have the last commit, it looks correct (though I would want CI to run to be sure).

One issue is that main has also updated the inheritance-diagram that you had to update, which is causing a conflict.

Unfortunately that is one big long line that makes it hard to tell what the differences are:

It was edited in #26874, which removed the BrokenBarHCollection, so both that and CLabelText should be removed.

The following should resolve it:

git rebase --onto upstream/main HEAD~1
# Resolve the conflict noted above, leaving that line in once, the git `>>>>`/`====`/`<<<<` lines removed, etc and both entries to be removed left out
$EDITOR doc/api/artist_api.rst
git add doc/api/artist_api.rst
git rebase --continue
# Edit commit message
git push --force-with-lease

If you'd rather we do it, I'm willing, but that is what I believe is needed.

@DHClimber
Copy link
Contributor Author

e is that main has also updated the

Thanks, I'll give it one more try. Thanks for the patience.

@QuLogic
Copy link
Member

QuLogic commented Oct 3, 2023

@DHClimber it appears something messed up here, as you have only a single newline added to one file now. I think the correct commit is f448c0f. You can re-fetch that with (assuming you have the upstream remote as we recommend):

$ git fetch upstream f448c0fc73a624edf46806db961392518b365358:original-depreciated

Then you can reset your branch to it:

$ git checkout depreciated
$ git reset --hard original-depreciated

and push to replace it here:

$ git push --force-with-lease origin depreciated

@DHClimber
Copy link
Contributor Author

@DHClimber it appears something messed up here, as you have only a single newline added to one file now. I think the correct commit is f448c0f. You can re-fetch that with (assuming you have the upstream remote as we recommend):

$ git fetch upstream f448c0fc73a624edf46806db961392518b365358:original-depreciated

Then you can reset your branch to it:

$ git checkout depreciated
$ git reset --hard original-depreciated

and push to replace it here:

$ git push --force-with-lease origin depreciated

Thank you, I have been out of town traveling. I'll try to fix now.

@DHClimber
Copy link
Contributor Author

It seems like correct commit now, but I'm confused what is causing automated tests to fail. If anyone can provide a hint I'm happy to try and correct it.

@melissawm
Copy link
Contributor

It looks like you have some conflicts - it may be that something didn't go right on your rebase. Could you try checking again? Here's relevant documentation, hope it helps: https://matplotlib.org/devdocs/devel/development_workflow.html#manage-commit-history

If you need extra help let us know.

@DHClimber DHClimber force-pushed the depreciated branch 3 times, most recently from e1219d8 to 998783e Compare October 10, 2023 02:54
@DHClimber
Copy link
Contributor Author

DHClimber commented Oct 10, 2023

It looks like you have some conflicts - it may be that something didn't go right on your rebase. Could you try checking again? Here's relevant documentation, hope it helps: https://matplotlib.org/devdocs/devel/development_workflow.html#manage-commit-history

If you need extra help let us know.

Sorry, I've tried everything I can think of but nothing is making sense. I can't understand where the conflicts are coming from, I'm just shooting in the dark now.

artist_api.rst error is complaining my branch doesn't match "main", but I don't understand why that is an error when the file needed to be updated?

@ksunden
Copy link
Member

ksunden commented Oct 10, 2023

I think I should have cleaned that up, I actually had a branch from the first time I advised a rebase sitting on one of my machines that I had taken care of the conflicts on, so I just rebased that again and pushed it up

@DHClimber
Copy link
Contributor Author

I think I should have cleaned that up, I actually had a branch from the first time I advised a rebase sitting on one of my machines that I had taken care of the conflicts on, so I just rebased that again and pushed it up

Thanks! I was so confused, didn't even know that was a possibility.

@QuLogic
Copy link
Member

QuLogic commented Oct 11, 2023

Can you update the commit message? It currently is an incomplete statement: 'removal of deprecated'. It would be better if it matched this PR title, for example.

@DHClimber
Copy link
Contributor Author

Can you update the commit message? It currently is an incomplete statement: 'removal of deprecated'. It would be better if it matched this PR title, for example.

Sorry about that, should be corrected now.

@QuLogic QuLogic added this to the v3.9.0 milestone Oct 11, 2023
@@ -16,7 +16,7 @@ from collections.abc import Callable, Iterable, Sequence
from typing import Literal
from .typing import ColorType

class ClabelText(Text): ...

Copy link
Member

Choose a reason for hiding this comment

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

Remove this line instead.

Suggested change

@DHClimber
Copy link
Contributor Author

Just following up, is there anything else that needs to be done here?

@QuLogic
Copy link
Member

QuLogic commented Nov 16, 2023

We require two approvals before merges, so this is waiting for a second reviewer.

@ksunden ksunden merged commit b16031c into matplotlib:main Nov 16, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants