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

Implement pre-commit & various tools #245

Merged
merged 30 commits into from
Nov 8, 2021

Conversation

CasperWA
Copy link
Contributor

@CasperWA CasperWA commented Sep 29, 2021

Description:

Closes #243.
Partly addresses #223, but does not fully close it. Now fully closes #223.

Implement pre-commit.
Pre-commit is a tool that one installs in the git repository and runs prior to committing, checking various parts of the changed files.
For more information go to pre-commit.com.

This slowly introduces pre-commit hooks.
First several hooks from the pre-commit-hooks repository maintained by pre-commit themselves are added:

  • check-symlinks
  • check-xml (running for all xml, rdf, and ttl files)
  • check-yaml
  • destroyed-symlinks
  • end-of-file-fixer
  • requirements-txt-fixer
  • trailing-whitespace

To understand what each of these hooks do, go the README for the pre-commit-hooks repo.

Then bandit and safety will be introduced.
bandit will be introduced both as a pre-commit hooks, as well as running in the CI.
safety will only run in the CI.

Type of change:

  • Bug fix.
  • New feature.
  • Documentation update.

Checklist:

This checklist can be used as a help for the reviewer.

  • Is the code easy to read and understand?
  • Are comments for humans to read, not computers to disregard?
  • Does a new feature has an accompanying new test (in the CI or unit testing schemes)?
  • Has the documentation been updated as necessary?
  • Does this close the issue?
  • Is the change limited to the issue?
  • Are errors handled for all outcomes?
  • Does the new feature provide new restrictions on dependencies, and if so is this documented?

Comments:

To do:

  • Setup pre-commit
  • Setup pre-commit-hooks hooks
  • Setup documentation build hooks (from invoke tasks)
  • Add bandit
  • Add safety
  • Add pylint (removing flake8)
  • Add black

@CasperWA
Copy link
Contributor Author

Issues solved with bandit (checkout the documentation for more information):

  • B101
  • B405
  • B310
  • B404
  • B314
  • B110

@CasperWA
Copy link
Contributor Author

Special pre-commit hooks have been added to update the documentation nicely using the invoke tasks.

@CasperWA
Copy link
Contributor Author

CasperWA commented Sep 29, 2021

This PR should also slightly address #241.
However, the "EMMO documentation" job should still be revisited, as well as the now out-commented lines for the "demos".

@CasperWA CasperWA force-pushed the cwa/close-243-223-pre-commit-linting-formatting branch 2 times, most recently from 3c45922 to af952a7 Compare October 14, 2021 11:09
@CasperWA CasperWA force-pushed the cwa/close-243-223-pre-commit-linting-formatting branch 2 times, most recently from 1021a98 to 7279f4f Compare October 26, 2021 12:34
@CasperWA CasperWA marked this pull request as ready for review October 26, 2021 13:41
@CasperWA CasperWA force-pushed the cwa/close-243-223-pre-commit-linting-formatting branch from 970f53f to c715e76 Compare October 28, 2021 11:35
@CasperWA CasperWA mentioned this pull request Nov 1, 2021
11 tasks
@CasperWA CasperWA force-pushed the cwa/close-243-223-pre-commit-linting-formatting branch 2 times, most recently from 31a1e52 to 5b67bf6 Compare November 2, 2021 12:22
CasperWA and others added 17 commits November 2, 2021 14:43
This adds a new installable "extra": `dev`.
`dev` installs all `docs` dependencies (the ones from
`requirements_docs.txt`) as well as all development-specific
dependencies (the ones from `requirements_dev.txt`).
These hooks include:
- Check symlinks
- Check XML (run only for .xml, .rdf, and .ttl files).
- Check YAML
- Destroyed symlinks
- End-of-file fixer
- Requirements*.txt fixer
- Trailing whitespace

To see what all the hooks do, go to
https://github.com/pre-commit/pre-commit-hooks
Use `defusedxml` instead of `xml`

Replace `assert` statements with `if` -> `raise`.

All notes related to `subprocess` have been suppressed.
It might be good to truly ensure no harmful commands can be run using
any of the `subprocess` commands at some point.

The found and treated codes include:
- B101
- B405
- B310
- B404
- B314
- B110

For more information see the `bandit` documentation at:
https://bandit.readthedocs.io
If any Python file in the `emmopy` or `ontopy` folders are changed, the
`invoke` task `create-api-reference-docs` is run, updating the API
Reference section of the documentation.

If the README.md file is changed, the `invoke` task `create-docs-index`
is run, updating the landing page for the documentation (´index.md`).
The pre-commit hook couldn't fail previously.
Now a `--pre-commit` option has been added to the `invoke` task that
runs `git status` and checks whether any changes are present and if they
have been staged (added to the git index).
If changes are present and they have *not* been staged, the hook will
fail, listing all the non-staged changes.
Otherwise it succeeds.

Ran the invoke task and updated the API Reference documentation.
Use `vMAJOR` version tags for appliccable actions.

Add a job to run `safety`.
This may also include running `pylint` in the future, due to `pylint`'s
aversion to running in a virtual environment via `pre-commit` due to
importability.

Comment out the "demo" steps of the `tests` job so that it only runs
`pytest` now - which is essentially equivalent to before.
Update `pre-commit` CI job accordingly, avoiding to install `pytest`.
Add a section under "Developers" in the documentation to document how to
setup a developer environment for EMMOntoPy and installing it for
development use.
Black is a stylizer, and as such is re-organizes, but does not change
code.

Gone through all changes, put multi-line strings into parentheses where
applicable.

Implemented very minor fixes (turned a stand-alone string into a
comment).
Update `pyproject.toml` config file with recommended setup from `black`:
https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#pylint
CasperWA and others added 11 commits November 2, 2021 14:43
Disable `no-member` in ontodoc and ontograph.
Generally disable `duplicate-code`.
Identified, corrected, and disabled `cyclic-import`s.
Include `tools` explicitly, since these Python files do not include the
`.py` file extension.
Issues with renaming `format` -> `fmt` and renaming `EMMObased` ->
`emmo_based`.
Disable `unspecified-encoding` for pylint.
Since it's now done within a `with`-statement, the `run_pandoc()`
function must also be called within that `with`-statement block.
No updates detected after running pre-commit with updated version.
@CasperWA CasperWA force-pushed the cwa/close-243-223-pre-commit-linting-formatting branch from 5604a00 to 384d844 Compare November 2, 2021 13:45
CasperWA and others added 2 commits November 8, 2021 09:54
Co-authored-by: Francesca L. Bleken <[email protected]>
Update `venv` section, remove `conda` section, and add `virtual env
considerations` tip.

Furthermore, use `$` in `console` code blocks to enhance coloring.
An alternative can be to use `shell` and leave out `$`.
@CasperWA CasperWA requested a review from francescalb November 8, 2021 11:03
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Lots of very good improvements, this will enhance the code development greatly I think.

@CasperWA CasperWA merged commit d5ae907 into master Nov 8, 2021
@CasperWA CasperWA deleted the cwa/close-243-223-pre-commit-linting-formatting branch November 8, 2021 13:49
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.

Use pre-commit Update code styling and linting
2 participants