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

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

Conversation

rishavganguly007
Copy link

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.

Copy link

@github-actions github-actions bot left a comment

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
Copy link
Member

QuLogic commented Mar 2, 2023

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
Contributor

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
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
Contributor

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.

@rishavganguly007
Copy link
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
Contributor

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
Copy link
Member

rcomer commented Jun 24, 2023

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

@rcomer
Copy link
Member

rcomer commented Jul 30, 2023

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.

None yet

4 participants