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

Clarify AxLine Params #27534

Merged
merged 1 commit into from Dec 17, 2023
Merged

Clarify AxLine Params #27534

merged 1 commit into from Dec 17, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Dec 17, 2023

PR summary

Closes #27503 following #27503 (comment).

PR checklist

@rcomer rcomer added the Documentation: API files in lib/ and doc/api label Dec 17, 2023
@rcomer rcomer added this to the v3.8.3 milestone Dec 17, 2023
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

I can't flag the type error cause it's not in this PR, but I get the feeling it's gonna complicate things here b/c it implies you only need to pass one.

lib/matplotlib/lines.py Outdated Show resolved Hide resolved
@rcomer
Copy link
Member Author

rcomer commented Dec 17, 2023

I can't flag the type error cause it's not in this PR, but I get the feeling it's gonna complicate things here b/c it implies you only need to pass one.

I'm sorry I'm not quite following this

@timhoffm timhoffm merged commit dce9ea4 into matplotlib:main Dec 17, 2023
29 of 30 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Dec 17, 2023
@rcomer rcomer deleted the axline-params branch December 17, 2023 20:09
@story645
Copy link
Member

story645 commented Dec 17, 2023

The error message that checks the parameters says that exactly one must be given. I think given is ambiguous enough in this context that one valid reading is that only one parameter must be passed in.

if (xy2 is None and slope is None or
xy2 is not None and slope is not None):
raise TypeError(
"Exactly one of 'xy2' and 'slope' must be given")

oscargus added a commit that referenced this pull request Dec 17, 2023
…534-on-v3.8.x

Backport PR #27534 on branch v3.8.x (Clarify AxLine Params)
@rcomer
Copy link
Member Author

rcomer commented Dec 18, 2023

@story645 thanks I’m with you now (my head was stuck in mypy type errors). The majority of users who see that error will be using ax.axline, so this error makes sense for them. Not sure what the best thing to do here is.

@story645
Copy link
Member

I might be missing a beat, why can't both parameters be defaulted to none in the init?

@oscargus
Copy link
Contributor

I came here to say the same as @story645.

(With that said, I may also be missing something here...)

However, not sure that such a change should be backported? And an API-change note is required.

(Also, one may consider documenting that **kwargs are forwarded to Line2D.)

@timhoffm
Copy link
Member

Fully agree that requiring xy2 and slope is not a good API. Ideally, we would have introduced a class method from_slope tor the latter. Though not sure that it's worth changing now.

I'm fine with introducing None-defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: API files in lib/ and doc/api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot Create lines.AxLine
4 participants