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

Adding black formatter pre-commit #403

Merged
merged 7 commits into from
Oct 31, 2023
Merged

Adding black formatter pre-commit #403

merged 7 commits into from
Oct 31, 2023

Conversation

nmerket
Copy link
Member

@nmerket nmerket commented Oct 24, 2023

Pull Request Description

Adding pre-commit and black code formatter.

Checklist

Not all may apply

  • Code changes (must work)
  • Tests exercising your feature/bug fix (check coverage report on Checks -> BuildStockBatch Tests -> Artifacts)
  • Coverage has increased or at least not decreased. Update minimum_coverage in .github/workflows/ci.yml as necessary.
  • All other unit and integration tests passing
  • Update validation for project config yaml file changes
  • Update existing documentation
  • Run a small batch run on Eagle to make sure it all works if you made changes that will affect Eagle
  • Add to the changelog_dev.rst file and propose migration text in the pull request

@nmerket nmerket requested a review from lixiliu October 24, 2023 18:37
@github-actions
Copy link

github-actions bot commented Oct 24, 2023

File Coverage
All files 86%
base.py 90%
eagle.py 77%
exc.py 57%
local.py 70%
postprocessing.py 84%
utils.py 96%
sampler/base.py 79%
sampler/downselect.py 33%
sampler/precomputed.py 93%
sampler/residential_quota.py 61%
test/test_docker.py 33%
test/test_validation.py 97%
workflow_generator/base.py 90%
workflow_generator/commercial.py 53%
workflow_generator/residential_hpxml.py 86%

Minimum allowed coverage is 33%

Generated by 🐒 cobertura-action against ba56460

@shorowit
Copy link
Contributor

Note that as an alternative to developers having to install pre-commit, there are services that will run the pre-commit automatically. Like this one. It's free and integrates with GitHub.

@shorowit
Copy link
Contributor

https://github.com/pre-commit/action even says:

"this action is in maintenance-only mode and will not be accepting new features.

generally you want to use pre-commit.ci which is faster and has more features."

Copy link
Contributor

@lixiliu lixiliu left a comment

Choose a reason for hiding this comment

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

LGTM. Though you might want to update bsb documentation to show how to activate the pre-commit hook.

@rajeee
Copy link
Contributor

rajeee commented Oct 30, 2023

Can we keep 120 line length and avoid excessive reformatting of the code base?

@nmerket
Copy link
Member Author

nmerket commented Oct 30, 2023

@rajee I'm open to it. It looks like to set a custom line length, I'd need to use a pyproject.toml. However, buildstockbatch is using setup.py. We'll need to convert the setup.py into a pyproject.toml. Would you be able to take a stab at that?

@nmerket nmerket merged commit 6618962 into develop Oct 31, 2023
6 checks passed
@nmerket nmerket deleted the black branch October 31, 2023 15:59
@rajeee
Copy link
Contributor

rajeee commented Oct 31, 2023

@rajee I'm open to it. It looks like to set a custom line length, I'd need to use a pyproject.toml. However, buildstockbatch is using setup.py. We'll need to convert the setup.py into a pyproject.toml. Would you be able to take a stab at that?

Glad you got it sorted without switching the whole project into pyproject.toml. I don't have a good experience with pyproject.toml - I tried using it for buildstock-query but had to revert back to setup.py because at that time, the toml version didn't support editable mode installation.

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.

4 participants