add initial precommit workflows #150
Conversation
|
@code-review-assist review |
![code-review-assist[bot]](https://avatars.githubusercontent.com/in/1008254?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request adds pre-commit workflows for linting and link checking. This is a great addition and will help maintain code quality and consistency. The chosen linters are appropriate for the project's languages. A few suggestions for improvement are below.
.github/workflows/linter.yaml
Outdated
| ############################# | ||
| on: | ||
| push: | ||
| branches-ignore: [main] |
There was a problem hiding this comment.
Consider linting against the main branch instead of ignoring it. This ensures any changes are compatible with the main branch before merging, preventing integration issues.
| branches-ignore: [main] | |
| branches: [main] |
.github/workflows/linter.yaml
Outdated
| env: | ||
| DEFAULT_BRANCH: main | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| FILTER_REGEX_EXCLUDE: 'gemini/sample-apps/finance-advisor-spanner/.*\.sql' |
There was a problem hiding this comment.
The FILTER_REGEX_EXCLUDE setting could be documented with a comment explaining its purpose and the files it excludes. This improves clarity and maintainability.
| FILTER_REGEX_EXCLUDE: 'gemini/sample-apps/finance-advisor-spanner/.*\.sql' | |
| # Exclude specific files from linting, e.g., SQL files in a specific directory. | |
| FILTER_REGEX_EXCLUDE: 'gemini/sample-apps/finance-advisor-spanner/.*\\.sql' |
.github/workflows/linter.yaml
Outdated
| VALIDATE_PYTHON_ISORT: false | ||
| VALIDATE_CHECKOV: false | ||
| VALIDATE_JAVASCRIPT_STANDARD: false | ||
| VALIDATE_TYPESCRIPT_STANDARD: false | ||
| VALIDATE_GITHUB_ACTIONS: false | ||
| VALIDATE_YAML_PRETTIER: false |
There was a problem hiding this comment.
Consider enabling VALIDATE_PYTHON_ISORT, VALIDATE_GITHUB_ACTIONS, and VALIDATE_YAML_PRETTIER for better code style consistency and catching potential issues in GitHub Actions and YAML files. While disabling some linters might be necessary for specific reasons, ensure these decisions are documented for clarity.
| VALIDATE_PYTHON_ISORT: false | |
| VALIDATE_CHECKOV: false | |
| VALIDATE_JAVASCRIPT_STANDARD: false | |
| VALIDATE_TYPESCRIPT_STANDARD: false | |
| VALIDATE_GITHUB_ACTIONS: false | |
| VALIDATE_YAML_PRETTIER: false | |
| VALIDATE_PYTHON_ISORT: true | |
| VALIDATE_GITHUB_ACTIONS: true | |
| VALIDATE_YAML_PRETTIER: true | |
| VALIDATE_CHECKOV: false | |
| VALIDATE_JAVASCRIPT_STANDARD: false | |
| VALIDATE_TYPESCRIPT_STANDARD: false |
| on: | ||
| pull_request: | ||
| branches: [main] | ||
| push: |
There was a problem hiding this comment.
Similar to the linter workflow, consider changing this to lint against the main branch to catch issues before merging.
| push: | |
| branches: [main] |
| else | ||
| # If running in a push | ||
| while read -r file || [ -n "$line" ]; do | ||
| notebooks+=("$file") |
There was a problem hiding this comment.
The comparison HEAD^1 might not be reliable in all cases (e.g., merged commits). Consider using origin/main... (or your default branch) to compare against the main branch, ensuring consistent behavior across different scenarios.
| notebooks+=("$file") | |
| done < <(git diff --name-only origin/main... | grep '\\.ipynb$') |
| python3 -m tensorflow_docs.tools.nbfmt --test "$notebook" | ||
| NBFMT_RTN=$? | ||
| echo "Running flake8..." | ||
| python3 -m nbqa flake8 "$notebook" --show-source --extend-ignore=W391,E501,F821,E402,F404,F704,W503,E203,E722,W293,W291,I100 |
There was a problem hiding this comment.
Consider adding more specific error codes to extend-ignore to provide more granular control over which flake8 errors are ignored. Documenting the reasons for ignoring specific errors can enhance maintainability.
| python3 -m nbqa flake8 "$notebook" --show-source --extend-ignore=W391,E501,F821,E402,F404,F704,W503,E203,E722,W293,W291,I100 | |
| python3 -m nbqa flake8 "$notebook" --show-source --extend-ignore=W391,E501,F821,E402,F404,F704,W503,E203,E722,W293,W291,I100 # Ignoring specific flake8 errors, add reasons here. |
No description provided.