FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc'] by NoyHanan · Pull Request #25839 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

[ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc'] - #25839

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
NoyHanan:bugfix-for-issue-22338
Jul 15, 2023
ConversationCommits (1)ChecksFiles changed
Merged

[ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc']#25839
QuLogic merged 1 commit into
matplotlib:mainfrom
NoyHanan:bugfix-for-issue-22338

Conversation

NoyHanan commented May 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

closes #22338

PR summary

I Added the ability to use int / float-tuple like kwarg legend(loc) for rcParams['legend.loc'], by creating a new validator for rcParams['legend.loc'].
This new change will help to a consistent and a uniform approach to using the legend.
Addresses both parts of the issue

PR checklist

NoyHanan marked this pull request as ready for review May 10, 2023 12:17
NoyHanan force-pushed the bugfix-for-issue-22338 branch from 5f21ef5 to 21a74f4 Compare May 11, 2023 05:12
chahak13 reviewed May 11, 2023
View reviewed changes
Comment thread lib/matplotlib/rcsetup.py Outdated

chahak13 commented May 11, 2023

Copy link
Copy Markdown
Contributor

Thank you for your first PR 🎉 Looking forward to more such contributions! :)

NoyHanan commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for your first PR 🎉 Looking forward to more such contributions! :)

Thank you so much!

NoyHanan commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

@chahak13 Do I need to note for the API change / add documentation?

chahak13 reviewed May 11, 2023
View reviewed changes
Comment thread lib/matplotlib/tests/test_rcparams.py Outdated

chahak13 commented May 11, 2023

Copy link
Copy Markdown
Contributor

There should be a note in the what's new section I believe, yes.

NoyHanan commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

There should be a note in the what's new section I believe, yes.

Ok, I'll take care of it tomorrow then.

tacaswell added this to the v3.8.0 milestone May 12, 2023
tacaswell reviewed May 12, 2023
View reviewed changes
Comment thread lib/matplotlib/rcsetup.pyi Outdated
tacaswell reviewed May 12, 2023
View reviewed changes
Comment thread lib/matplotlib/tests/test_rcparams.py Outdated
tacaswell reviewed May 12, 2023
View reviewed changes
Comment thread doc/api/next_api_changes/behavior/22338-NH.rst Outdated

tacaswell commented May 12, 2023

Copy link
Copy Markdown
Member

Thank you for your work on this @NoyHanan , I think it is headed the right direction! However I left a couple of comments that need to be addressed.

NoyHanan commented May 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you for your work on this @NoyHanan , I think it is headed the right direction! However I left a couple of comments that need to be addressed.

Hi @tacaswell and @ksunden ,
Thank you for taking the time to review my PR. I appreciate your feedback and will begin working on making the necessary changes.

tacaswell commented May 16, 2023

Copy link
Copy Markdown
Member

There should also be a test of reading an the values from a file.

NoyHanan commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

There should also be a test of reading an the values from a file.

On it.

NoyHanan commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

Hi @tacaswell!
I know that you many notifications to attend to, so I wanted to kindly inform you that I have made the necessary adjustments to the code based on your feedback (or at least I believe I have :) ).

NoyHanan commented Jun 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @tacaswell! I know that you many notifications to attend to, so I wanted to kindly inform you that I have made the necessary adjustments to the code based on your feedback (or at least I believe I have :) ).

rcomer reviewed Jun 18, 2023
View reviewed changes

rcomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for your work on this @NoyHanan, and apologies for the delay in reviewing. Could you update the title of this PR to something more descriptive?

Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/tests/test_rcparams.py
NoyHanan changed the title Bugfix for issue 22338 [ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc'] Jun 21, 2023

NoyHanan commented Jun 21, 2023

Copy link
Copy Markdown
Contributor Author

Hi @rcomer,
I appreciate your thorough review of my code. I've made the necessary corrections based on your feedback. I'm looking forward to your thoughts on the updates. Thank you!

QuLogic reviewed Jun 24, 2023
View reviewed changes
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread doc/users/next_whats_new/rcParams[legend.loc]_supports_float_tuple.rst Outdated
Comment thread doc/users/next_whats_new/rcParams[legend.loc]_supports_float_tuple.rst Outdated
rcomer approved these changes Jun 24, 2023
View reviewed changes

rcomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @NoyHanan, your updates look good to me.

NoyHanan commented Jun 24, 2023

Copy link
Copy Markdown
Contributor Author

Thanks @NoyHanan, your updates look good to me.

@rcomer thanks alot! a few more things:

  1. I'm commiting the changes @QuLogic, does it need to be approved again?
  2. who may merge the pull request to the main repo?

rcomer commented Jun 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

For code changes, we require approvals from two core developers. So yes, please address @QuLogic's comments. Then once he is happy he will likely approve and merge.

NoyHanan force-pushed the bugfix-for-issue-22338 branch 2 times, most recently from 9bb4013 to df2ac49 Compare June 24, 2023 21:28

QuLogic commented Jun 28, 2023

Copy link
Copy Markdown
Member

It looks like you'll also need to update the typing stubs now that the validator is private. Also, you may want to squash the changes together.

NoyHanan commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

It looks like you'll also need to update the typing stubs now that the validator is private. Also, you may want to squash the changes together.

Sure, no problem.
Can you explain a bit more about how to update?

ksunden commented Jun 28, 2023

Copy link
Copy Markdown
Member

Just undo the change to the .pyi file

NoyHanan force-pushed the bugfix-for-issue-22338 branch from df2ac49 to 3e45dd2 Compare June 28, 2023 09:50

NoyHanan commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

Just undo the change to the .pyi file

Thanks!

rcomer commented Jun 28, 2023

Copy link
Copy Markdown
Member

To squash, you can do an interactive rebase. I strongly recommend saving a backup copy of your branch first if you have not done this (much) before.

NoyHanan commented Jul 8, 2023

Copy link
Copy Markdown
Contributor Author

HI @QuLogic, I'll appriciate your review on the changes I made 🙏

rcomer commented Jul 8, 2023

Copy link
Copy Markdown
Member

@NoyHanan are you happy to squash the commits, or would you prefer us to take care of that?

NoyHanan commented Jul 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@NoyHanan are you happy to squash the commits, or would you prefer us to take care of that?

@rcomer I can take care of that, I just thought you guys meant I should squash only the last 3 commits I made for @QuLogic notes.
Should I sqaush them all? and thank you so much for your help and patience.

rcomer commented Jul 8, 2023

Copy link
Copy Markdown
Member

I think minimal numbers of commits are generally preferred.

NoyHanan force-pushed the bugfix-for-issue-22338 branch from 3e45dd2 to 063c22e Compare July 9, 2023 09:33

NoyHanan commented Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

I think minimal numbers of commits are generally preferred.

Thank you for guiding me. I hope that its fine now.

NoyHanan commented Jul 15, 2023

Copy link
Copy Markdown
Contributor Author

Hi @QuLogic, could you please review the changes I made? 🙏

QuLogic merged commit c756cbd into matplotlib:main Jul 15, 2023

rcomer commented Jul 17, 2023

Copy link
Copy Markdown
Member

Congratulations on your first PR merged into Matplotlib @NoyHanan! We hope to hear from you again.

NoyHanan commented Jul 17, 2023

Copy link
Copy Markdown
Contributor Author

Congratulations on your first PR merged into Matplotlib @NoyHanan! We hope to hear from you again.

Thank you so much, I appreciate your support. You'll definitely hear from me again :)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments
ksunden ksunden left review comments
QuLogic QuLogic approved these changes
rcomer rcomer approved these changes
+1 more reviewer
chahak13 chahak13 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic: rcparams

Projects

First Time Contributors
Status: Merged

Milestone

v3.8.0

Development

Successfully merging this pull request may close these issues.

[Bug]: rcParams['legend.loc'] can't use float-tuple like kwarg legend(loc...)

7 participants

Back | FazBrowse Home | New Git URL