-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add CI gh-actions and pre-commit hook #181
base: development
Are you sure you want to change the base?
Conversation
eltonn
commented
Nov 25, 2020
- Resolves Add CI gh-actions and pre-commit hook #179
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.
thanks for working on that. I add some comments here.
.github/workflows/python-app.yml
Outdated
|
||
on: | ||
push: | ||
branches: [ development ] |
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.
it should be [ development, master ]
.github/workflows/python-app.yml
Outdated
push: | ||
branches: [ development ] | ||
pull_request: | ||
branches: [ development ] |
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.
it should be [ development, master ]
.github/workflows/python-app.yml
Outdated
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Set up Python 3.8 | ||
uses: actions/setup-python@v2 |
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.
instead of actions/setup-python@v2,
use conda-incubator/setup-miniconda@v2
ref: https://github.com/Quansight/ibis-vega-transform/blob/master/.github/workflows/test.yml#L22
note 1: check all the parameters.
note2: you need always to specify the shell when you want to activate the conda environment in your step: https://github.com/Quansight/ibis-vega-transform/blob/master/.github/workflows/test.yml#L27
.github/workflows/python-app.yml
Outdated
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install flake8 |
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.
install requirements using the conda-incubator configuration: https://github.com/Quansight/ibis-vega-transform/blob/master/.github/workflows/test.yml#L25
.github/workflows/python-app.yml
Outdated
- name: Lint with flake8 | ||
run: | | ||
# stop the build if there are Python syntax errors or undefined names | ||
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics |
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.
instead of call flake8
directly, call pre-commit-hook run --all-files
all the parameters should be used from the configuration files.
max-line-length should be 79 in the configuration file.
@@ -54,7 +55,8 @@ def episem(x, sep='W', out='YW'): | |||
""" | |||
Return Brazilian corresponding epidemiological week from x. | |||
|
|||
:param x: Input date. Can be a string in the format %Y-%m-%d or datetime.datetime | |||
:param x: Input date. Can be a string in |
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.
note: maybe in the future, it would be good to use numpy doc style for the docstrings. no change is needed now.
@@ -32,17 +35,17 @@ | |||
# version is used. | |||
sys.path.insert(0, project_root) | |||
|
|||
import fludashboard |
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.
keep the import here and add a noqa
comment for now.
- conda-forge | ||
- defaults | ||
dependencies: | ||
- flask |
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.
note: check if it is the same as https://github.com/conda-forge/fludashboard-feedstock/blob/master/recipe/meta.yaml#L24
if it is not the same, consider to change it here or if your changes is necessary, consider to open a PR to update the feedstock.
environment.yml
Outdated
- defaults | ||
dependencies: | ||
- flask | ||
- pandas>=0.21 |
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.
add space after the name of the lib, eg: pandas >=0.21
environment.yml
Outdated
dependencies: | ||
- flask | ||
- pandas>=0.21 | ||
- plotly==2.5.1 |
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.
you don't need to add ==
here, eg: - plotly 2.5.1
.github/workflows/python-app.yml
Outdated
# This workflow will install Python dependencies, run tests and lint with a single version of Python | ||
# For more information see: https://help.github.com/actions/language-and-framework-guides/using-python-with-github-actions | ||
|
||
name: run enviroment and lint on fludashborad application |
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.
just one word for the name, it could be for example test
- name: Checkout branch | ||
uses: actions/checkout@v2 | ||
- name: Setup conda | ||
uses: conda-incubator/setup-miniconda@v2 |
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.
note: version 2.0.1 was released
.github/workflows/python-app.yml
Outdated
jobs: | ||
build: | ||
runs-on: ubuntu-latest | ||
env: |
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.
probably you can remove this env