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

Rewrite and refactor all of pycoast to meet minimum modern standards #63

Merged
merged 29 commits into from
Aug 15, 2022

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented May 11, 2022

This PR makes every change necessary to make pycoast pass basic formatting, linting, and complexity checks. To be sure of that I have enabled pre-commit.ci and added a pre-commit config that can be used by developers to ensure the quality of the code moving forward. I am almost done with the flake8 complexity issues and then there are a few bandit security issues I need to look into.

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff
  • Fully documented

@coveralls
Copy link

coveralls commented May 11, 2022

Coverage Status

Coverage increased (+1.0%) to 95.386% when pulling 9ffe62c on djhoese:refactor-black into b2ad512 on pytroll:main.

@djhoese djhoese marked this pull request as ready for review May 12, 2022 19:11
@djhoese
Copy link
Member Author

djhoese commented May 12, 2022

This could still definitely use a lot more work, but I've reduce the complexity a lot for the entire cw_base.py module. Some of it still isn't pretty, but I think it requires considering changing the algorithm. The hard part for handling the grid lines was that the minor lines depend on what the major lines are and a couple other odd combinations of things. This made it hard to separate the code further.

@lobsiger CodeFactor is finally happy 😉

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Ok, I got tired of writing the same comment over and over, I think black should be fixed to allow 120 characters and not split stuff randomly.

@djhoese
Copy link
Member Author

djhoese commented May 13, 2022

Ok, your comments have been addressed. I mentioned pytest in the test document. I updated black from using its default 88 character line length to 120.

@mraspaud
Copy link
Member

Ok, your comments have been addressed. I mentioned pytest in the test document. I updated black from using its default 88 character line length to 120.

Great, I can continue reviewing then :) Thanks!!!

djhoese added 2 commits May 13, 2022 10:57
This should allow anyone using black (pre-commit or not) to have the same behavior
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM. Mixing the refactoring with "blackification" didn't help either, maybe the next time we do this, we should do two separate PRs? But from what I can see, the tests didn't change in functionality, so I think we're good.

@djhoese
Copy link
Member Author

djhoese commented Aug 15, 2022

Yes, agreed. This started as a "add pre-commit" PR but to include all of the hooks I wanted it required rewriting most of the package. And yes, no behavior was intentionally changed. Merging now and then I could start working toward a config interface and automatically download the coastline files.

@djhoese djhoese merged commit 067789b into pytroll:main Aug 15, 2022
@djhoese djhoese deleted the refactor-black branch August 15, 2022 13:39
@lobsiger
Copy link
Contributor

@djhoese @mraspaud now that this refactoring PR is merged, do we have a chance to see a Pycoast V. 1.6.0 anytime soon?

@djhoese
Copy link
Member Author

djhoese commented Aug 19, 2022

@lobsiger I'm currently finishing my recovery from COVID and will need to submit a final report for one of my projects next week then catch up on all my other work. After all that, yes I'd like to look into a release. However, I noticed in the newest pre-commit PR #64 that one of the tests is failing with the new versions of dependencies. This test wasn't failing when this PR last ran it's CI. Debugging this test failure is the first major hurdle to getting a new release out. If you have any free time to investigate it would be much appreciated. Just narrowing the problem down to what versions of what dependencies causes the failure would be huge.

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

Successfully merging this pull request may close these issues.

4 participants