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

Github action setup initiative #788

Merged
merged 5 commits into from
Nov 3, 2021
Merged

Github action setup initiative #788

merged 5 commits into from
Nov 3, 2021

Conversation

lee1043
Copy link
Contributor

@lee1043 lee1043 commented Nov 3, 2021

Copied from xcdat repo

@lee1043
Copy link
Contributor Author

lee1043 commented Nov 3, 2021

I guess having this .pre-commit-config.yaml should help some checks, at least for flake8. I copied the file from the xcdat repo.

@tomvothecoder @acordonez could you please see if this looks okay to you?

@lee1043
Copy link
Contributor Author

lee1043 commented Nov 3, 2021

Pre-commit test is failing because it expects json files to follow python style... Not sure how to tell pre-commit check to ignore *.json files..

@acordonez
Copy link
Collaborator

@lee1043 I found some info about flake8 in pre-commit hooks here: https://flake8.pycqa.org/en/latest/user/using-hooks.html. Might be able to add something like "exclude: *.json"?

Comment on lines 13 to 21
- repo: https://github.com/psf/black
rev: 21.9b0
hooks:
- id: black

- repo: https://github.com/timothycrosley/isort
rev: 5.9.3
hooks:
- id: isort
Copy link
Collaborator

@tomvothecoder tomvothecoder Nov 3, 2021

Choose a reason for hiding this comment

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

black is a code formatter and isort is an import statement sorter.

I'm assuming the codebase did not integrate these QA tools previously. This means both of these tools (along with the other hooks) need to be run on the codebase and perform formatting as needed. Otherwise, pre-commit will most likely throw error(s).

There are a few options moving forward

  1. Run pre-commit locally and format everything in this PR
    • This will produce a large diff regardless, so this might be the way to go
    • Local usage: pre-commit install and pre-commit run --all-files
  2. Exclude these for now and open up another issue if you want to use these QA tools
  3. Exclude them and don't use them (I personally prefer to use them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomvothecoder many thanks for the info. I think your guess is correct, that such tests have not been performed for the PMP codes. I think option 2 might be the most efficient way to move forward.

Comment on lines +25 to +37
- repo: https://github.com/pycqa/flake8
rev: 3.9.2
hooks:
- id: flake8
args: ["--config=setup.cfg"]
additional_dependencies: [flake8-isort]
exclude: ^sample_setups/jsons/

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.910
hooks:
- id: mypy
args: ["--config=setup.cfg"]
Copy link
Collaborator

@tomvothecoder tomvothecoder Nov 3, 2021

Choose a reason for hiding this comment

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

Same comment regarding running the tools and fixing issues.

mypy is an optional static type checker. flake8 and mypy may catch some existing issues that need to be fixed manually.

Comment on lines +5 to +11
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.4.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are general QA hooks

@tomvothecoder
Copy link
Collaborator

Pre-commit test is failing because it expects json files to follow python style... Not sure how to tell pre-commit check to ignore *.json files..

pre-commit is failing due to the Trim Trailing Whitespace hook. https://github.com/PCMDI/pcmdi_metrics/runs/4094320066?check_suite_focus=true#step:4:70
I expect the other hooks to also fail based on my review comments.

Configuring the QA tools/hooks:

@lee1043 lee1043 merged commit e487a9e into master Nov 3, 2021
@lee1043 lee1043 deleted the 787_ljw_pre-commit-hook branch November 3, 2021 20:51
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.

3 participants