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

Remove and deprecate unused methods in src #25728

Merged
merged 3 commits into from May 10, 2023
Merged

Conversation

oscargus
Copy link
Contributor

@oscargus oscargus commented Apr 20, 2023

PR Summary

These are not used based on code coverage and grepping. Also, they are not really documented publicly as far as I can see. The ones that are directly removed are not even exported to any public object.

Will add notes for the deprecations, but wanted to get some initial feedback first.

(Edit: there is more unused/uncovered code. Some operators are not currently used, and some debug functionality is, naturally, not covered. To me it made sense to keep those. The ones in this PR should be safe to remove though. For the deprecations, one cannot really know. Hence, the specific message.)

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Comment on lines 96 to 97
"--\n\n"
".. deprecated:: 3.8\n";
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this become matplotlib._path, thus private, so we don't need a deprecation for anything in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it, but wasn't fully sure if that was considered "fully private" as it starts with _ or if it starts with _ just because it is a C-module (I seem to recall some discussion about that from about a year ago).

But it is for sure not publicly documented. Guess I was just being overly kind to people that may have used it (since it is exported).

Copy link
Member

Choose a reason for hiding this comment

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

In this case we have matplotlib.path, so this is definitely not directly what should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! Should there be a change note for it?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not, I think.

src/ft2font.cpp Show resolved Hide resolved
@oscargus oscargus force-pushed the srccleanup branch 2 times, most recently from c9e1350 to 3f8146b Compare April 27, 2023 06:55
@oscargus
Copy link
Contributor Author

oscargus commented Apr 27, 2023

Is there even a point in keeping Ft2Image public? It is only used internally in one location _mathtext, although one can still import it from mathtext as well.

Edit: No, one cannot import from mathtext, but it is in the stub-file for mathtext for unknown reasons.

@ksunden
Copy link
Member

ksunden commented Apr 27, 2023

It was removed from mathtext.py in #24984, but the type hint was not updated/not yet merged to be updated, depending on ordering of those PRs

@scottshambaugh
Copy link
Contributor

@oscargus Is there a tool you used to automate finding this dead code? I'm interested in running a similar process on some of my other repos. Was reading this blog post the other day on Google's efforts to clean up orphaned code and got inspired.

@oscargus
Copy link
Contributor Author

oscargus commented May 3, 2023

@scottshambaugh Here, I've just used the code coverage and started from there. I guess a valid way would be to search for function/method definitions and then grep for that name in the code base (which is typically what I do once I've identified a potential candidate based on code coverage). Still require manual judgement, but I guess that is the main problem anyway: to know if it is just unused internally or if it is meant for "external" use. (For private functions/methods, it is less of a problem.)

My experience is that some static code checkers were able to do this pretty well. But primarily for other languages than Python. (I also guess it is different when you are a library compared to an application, when being active with JabRef, I relied on those tools much more than now, primarily contributing to MPL and SymPy.)

One thing I've been thinking about is if there are private methods that are tested directly (although it is not the encouraged approach) and therefore are covered, but otherwise unused?

Personally, I sometimes just try to get code coverage of things and in that way one can sometimes realize that some code really cannot be reached. Although it is not always clear where the problem is. For example, in SymPy I've not been able to reach code that clearly is handling a special case, just unknown which, and then it is not so clear if I just cannot figure out which case or if the case is now handled elsewhere. I think the corresponding cases in Matplotlib are probably things like error handling and array-conversions that may have moved elsewhere over time. When I first started contributing, I managed to find a few of those cases by simply trying to increase code coverage.

Hence, my conclusion is that one should have code coverage for all cases to start with and that the code coverage tools should report "unrelated" changes as well, to be able to detect that once modifying something.

Maybe obvious things, but at least my experience from a hobbyist perspective...

@scottshambaugh
Copy link
Contributor

Thank you for the info! The focus on code coverage makes sense, that seems like a good guide for finding where these orphaned code is a liability and not just a clutter.

@ksunden ksunden merged commit e4a7ef3 into matplotlib:main May 10, 2023
38 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants