Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

added raise condition and grouped Tick constructor parameters#26037

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

Open
madeira-dev wants to merge5 commits intomatplotlib:main
base:main
Choose a base branch
Loading
frommadeira-dev:refact_kwargs_tick

Conversation

madeira-dev
Copy link
Contributor

PR summary

closes#26008
Grouped constructor parameters as described in issue

def __init__(        self, axes, loc, *,        size=None, width=None, color=None, major=True, pad=None, zorder=None,  # global        tickdir=None, tick1On=True, tick2On=True,  # tick prop        labelsize=None, labelcolor=None, labelrotation=0,  # label prop        label1On=True, label2On=False,  # label prop        grid_color=None, grid_linestyle=None, grid_linewidth=None,  # grid prop        grid_alpha=None, gridOn=None,  # grid prop        **kwargs,  # Other Line2D kwargs applied to gridlines.    ):

and created raise condition

remaining_kwargs = {            k: v for k, v in kwargs.items() if not k.startswith('grid_')}        if remaining_kwargs:            raise ValueError(f"Invalid kwargs: {remaining_kwargs.keys()}")

PR checklist

remaining_kwargs = {
k: v for k, v in kwargs.items() if not k.startswith('grid_')}
if remaining_kwargs:
raise ValueError(f"Invalid kwargs: {remaining_kwargs.keys()}")
Copy link
Member

Choose a reason for hiding this comment

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

This probably should start as a warning for a cycle? On one hand I hope these is no usage of this in the wild (it is not documented and users should not be makingTick directly!), but on the other we tend to be very conservative about this sort of thing.

Eitherway, this needs an API change note.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Where and what should I write about in the API change note?
Should I write about changing it to raise in the next cycle?

@tacaswelltacaswell added this to thev3.8.0 milestoneJun 1, 2023
Copy link
Member

@timhoffmtimhoffm left a comment
edited
Loading

Choose a reason for hiding this comment

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

@tacaswell IMHO this does not need an API-change note and could even directly error instead of warn. There is no reasonable code that is currently working and will break through the change. One would have to have passed a different 5-char prefix such asGRID_alpha /gridsalpha /line_alpha. But all of that would be completely made up by the user. That this works is just a current implementation detail and we haven't documented or provided any guarantees on it.

grid_linestyle=None,
grid_linewidth=None,
grid_alpha=None,
size=None, width=None, color=None, major=True, pad=None, zorder=None, # global
Copy link
Member

@timhoffmtimhoffmAug 4, 2023
edited
Loading

Choose a reason for hiding this comment

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

The size in points info must stay.

I'd have a slight preference to keep the one-parameter-per-line format and reorder in there:

# globalsize=None,  # pointswidth=None,...# tick propstickdir=None,...

But that's personal taste and not a blocker.

Copy link
Member

Choose a reason for hiding this comment

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

I agree about the size in points comment staying.

@ksundenksunden modified the milestones:v3.8.0,v3.9.0Aug 8, 2023
@tacaswell
Copy link
Member

I will defer to@timhoffm here and skip the warning/deprecation cycle, but the text of the error raised should explain what is going on and what changed rather than just saying "invalid".

@timhoffm
Copy link
Member

@madeira-dev Do you want to continue on this?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

@timhoffmtimhoffmtimhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

[MNT]: Refactor kwargs of Tick constructor
5 participants
@madeira-dev@tacaswell@timhoffm@QuLogic@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp