-
Notifications
You must be signed in to change notification settings - Fork 1
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
Cif 315 improve consistency and standardization of cif code for layers #96
Cif 315 improve consistency and standardization of cif code for layers #96
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me other than the comments I made.
I've decided that I don't want to go further down the rabbit hole on the formatting topic. My main criteria with these changes were to:
I've already tested the code in this PR so am reluctant to make further re-formatting changes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but just curious, did you change all this manually? One thing I never set up here we have in most other repos is pre-commit, which can run a bunch of automatic linters on commit that'll do most of this for you.
Here's an example of a pre-commit config we have with linters like isort, black, and flake8 that'll fix spacing, ensure you're following PEP, etc.: https://github.com/wri/gfw-data-api/blob/master/.pre-commit-config.yaml
Your changes mostly look correct, but slightly different than the formatting I've seen in other repos.
@jterry64 I strongly agree that an automated, consistent reformatting would be best. I tried reformatting with VS Black and the default formatter in Pycharm but they didn't seem to do much. I'll follow-up with your suggestion above. Thanks |
How was this patch tested?
Generated layer files with prior and updated code and compared. Results were identical as measured with Meld.