Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix argument types in examples and tests#28764
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -509,7 +509,7 @@ def test_invalid_figure_add_axes(): | |||
| fig.add_axes((.1, .1, .5, np.nan)) | |||
| with pytest.raises(TypeError, match="multiple values for argument 'rect'"): | |||
| fig.add_axes([0, 0, 1, 1], rect=[0, 0, 1, 1]) | |||
| fig.add_axes((0, 0, 1, 1), rect=[0, 0, 1, 1]) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
rect should also be a tuple, though I'm surprised you don't get a type error for invalid parameters here (though that's on purpose and should be ignored.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is because I identify one type of error and then do regex-based search and replace for that on all files.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/tests/test_usetex.py Outdated
| @@ -168,7 +168,7 @@ def test_rotation(): | |||
| mpl.rcParams['text.usetex'] = True | |||
| fig = plt.figure() | |||
| ax = fig.add_axes([0, 0, 1, 1]) | |||
| ax = fig.add_axes((0, 0, 1, 1)) | |||
| ax.set(xlim=[-0.5, 5], xticks=[], ylim=[-0.5, 3], yticks=[], frame_on=False) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ax.set probably won't type check, but for consistency,xlim andylim here should also be tuples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I approve, subject to addressing@QuLogic's comments.
Will sort the comments out and over time find and fix more if this type when bored. |
I take the liberty of putting this one in draft since it seems to be contingent on@oscargus' boredom levels. |
I am also in favor of these changes, of course listswork, but don't allow you to specify length, which is why the type hints are Tuple... since the number of elements is super important for these cases |
I took the liberty to rebase and fix the commented issues. |
PR summary
Not sure if this is wanted, so I have only done this for a few methods as a discussion opener.
Background: I got a typing error in my editor when looking at an example.
It seems to me that if we type something as a tuple (and not as a tuple or a list), it makes sense to provide a tuple in all examples (and tests as I did some search-and-replace). But then I do not know the background to typing it as a tuple only.
PR checklist