Skip to content

Bug fix for Hatch linewidths in an rcParam context #21325 - #25367

Closed
rishavganguly007 wants to merge 6 commits into
matplotlib:mainfrom
rishavganguly007:bugfix/rishavganguly007/Hatch_Linewidths_#21108
Closed

rishavganguly007 wants to merge 6 commits into
matplotlib:mainfrom
rishavganguly007:bugfix/rishavganguly007/Hatch_Linewidths_#21108

Conversation

@rishavganguly007

Copy link
Copy Markdown

PR Summary

Provided fix based on the sugesstions given for the issue #21108

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@QuLogic

QuLogic commented Mar 2, 2023

Copy link
Copy Markdown
Member

Thank you for the PR. Please revert all the unrelated stylistic changes, as it is difficult to pin down what exactly you are changing here and they are all superfluous. This will also require adding some tests, I think.

@melissawm

Copy link
Copy Markdown
Member

Hi @rishavganguly007 - there are still a couple of lines that are too long to pass our linter, could you check that so we can move forward with the review? Thanks!

@rishavganguly007

Copy link
Copy Markdown
Author

Hi @rishavganguly007 - there are still a couple of lines that are too long to pass our linter, could you check that so we can move forward with the review? Thanks!

Hi @melissawm I have corrected the linting issue , but in the github test I can see still the flake8 test is failing for some whitespaces,
However, in my local when I am running the cmd flake8 <file path> I am not getting any issues.

@melissawm

Copy link
Copy Markdown
Member

Might be worth checking your flake8 installation - when I run flake8 lib/matplotlib/hatch.py locally, I also get

lib/matplotlib/hatch.py:239:1: W293 blank line contains whitespace
lib/matplotlib/hatch.py:239:38: W292 no newline at end of file

In any case, you can use the CI as the source of truth here and fix those anyway.

@rcomer rcomer mentioned this pull request Mar 11, 2023
2 of 7 tasks
@rishavganguly007

Copy link
Copy Markdown
Author

Might be worth checking your flake8 installation - when I run flake8 lib/matplotlib/hatch.py locally, I also get

lib/matplotlib/hatch.py:239:1: W293 blank line contains whitespace
lib/matplotlib/hatch.py:239:38: W292 no newline at end of file

In any case, you can use the CI as the source of truth here and fix those anyway.

Thanks @melissawm , all the lint related issues has been fixed.

@melissawm

Copy link
Copy Markdown
Member

Ok - it looks like now you have a few failed tests, can you take a look and see if those are related to your PR?

@rcomer

rcomer commented Jun 24, 2023

Copy link
Copy Markdown
Member

@rishavganguly007 are you still interested in working on this one?

@rcomer

rcomer commented Jul 30, 2023

Copy link
Copy Markdown
Member

I am going to close this one as it seems to have stalled. Closing will indicate to other contributors that they can pick up the issue. The patch is at #21108 (comment), so I think it will be at least as easy to start from there as it would be to rebase this branch.

@rishavganguly007 if you are still interested in working on this, please do comment and we can re-open.

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

Projects

Development

Successfully merging this pull request may close these issues.

4 participants