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

Cif 315 improve consistency and standardization of cif code for layers #96

Conversation

kcartier-wri
Copy link
Contributor

How was this patch tested?

Generated layer files with prior and updated code and compared. Results were identical as measured with Meld.

@kcartier-wri kcartier-wri marked this pull request as ready for review December 13, 2024 23:44
Copy link
Contributor

@chrowe chrowe left a 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.

city_metrix/layers/albedo.py Show resolved Hide resolved
city_metrix/layers/albedo.py Show resolved Hide resolved
@kcartier-wri
Copy link
Contributor Author

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:

  1. make the code more easily comprehensible by expanding functional code that was previously specified in one line into multiple lines with expressive indentation. We shouldn't obsess over the specifics - the code should just be easily readable, even with some stylistic inconsistency.
  2. remove complex return statements
  3. eliminate unused imports
  4. use meaningful variable names
  5. etc.

I've already tested the code in this PR so am reluctant to make further re-formatting changes,

Copy link
Member

@jterry64 jterry64 left a 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.

@kcartier-wri
Copy link
Contributor Author

@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

@kcartier-wri kcartier-wri merged commit fbc5a58 into main Dec 17, 2024
1 check passed
@kcartier-wri kcartier-wri deleted the CIF-315-Improve-consistency-and-standardization-of-CIF-code-for-Layers branch December 17, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants