Skip to content

Conversation

@csc-felipe
Copy link
Contributor

Description

Add pre-commit hooks and checks.

Also added a github action. If it works, and others agree, we could change a bit the github workflows and tox configuration.

Update your environment and enable the git hooks by running:

pip install -Ue .[docs,test,dev]
pre-commit install

Related issues

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

  • Added pre-commit configuration
  • fixed raised issues from the checks
  • moved pyspelling configuration to root folder
  • use ruff instead of flake8 and isort

Testing

  • Tests do not apply

Mentions

It might be easier to review by looking at commits individually.

I tried to use the pyupgrade fixes from ruff, but that broke the tests, and I didn't want to investigate. There are a few other tests that might be interesting to consider in the future: e.g. tryceratops, which checks for how to handle exceptions.

@csc-felipe
Copy link
Contributor Author

Weird that mypy has different output when running with pre-commit

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

not sure I understand the need for both tox and pre-commit workflows

@blankdots
Copy link
Contributor

pre-commit run --all-files -c .pre-commit-config.yaml gives

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

swift_browser_ui/request/db.py:48:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/request/db.py:51:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/request/db.py:54:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/sharing/db.py:53:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/sharing/db.py:56:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/sharing/db.py:59:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/ui/middlewares.py:74:11: C901 `error_middleware` is too complex (14 > 10)
Found 7 errors.

Maybe we should fix those as well ?

@csc-felipe
Copy link
Contributor Author

not sure I understand the need for both tox and pre-commit workflows

pre-commit doesn't run pytest. And tox is good for running the same tests with different environment configuration. Also, keeping it there might show issues with the pre-commit configuration, as it's happening now.

@csc-felipe
Copy link
Contributor Author

pre-commit run --all-files -c .pre-commit-config.yaml gives

Did you run pre-commit autoupdate? Those checks are in a new version of ruff, and I kept the same version from the previous PR so I wouldn't have to fix those now.

@blankdots
Copy link
Contributor

Did you run pre-commit autoupdate? Those checks are in a new version of ruff, and I kept the same version from the previous PR so I wouldn't have to fix those now.

yes i did :), well i don't see why not update

@csc-felipe
Copy link
Contributor Author

Weird that mypy has different output when running with pre-commit

If I remove installing requirements.txt from tox, mypy doesn't complain. Adding it to pre-commit config doesn't work

@blankdots
Copy link
Contributor

If I remove installing requirements.txt from tox, mypy doesn't complain. Adding it to pre-commit config doesn't work

i think that is not the solution, as mypy is correct to complain at that line, i think the problem is in pre-commit, I saw some something related to: https://github.com/pre-commit/mirrors-mypy/issues/3

removing additional_dependencies and adding language system seems to show the errors, but the additional dependencies need to be installed separately

Base automatically changed from feature/vue3 to devel March 28, 2023 07:28
@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch from 998e92e to 6c94057 Compare March 28, 2023 07:32
@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch from 6c94057 to 3bbc83c Compare March 28, 2023 07:34
@csc-felipe
Copy link
Contributor Author

Rebased and fixed the mypy issue. Now it runs from locally installed mypy, and added type deps to pyproject.toml

@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch 2 times, most recently from f270105 to 79f9fb4 Compare March 28, 2023 07:43
@blankdots
Copy link
Contributor

i am ok with the changes, but tests do need to work again.

I remember @csc-jm was working on them, maybe he is interested in this PR

@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch from 79f9fb4 to f5d22a4 Compare March 28, 2023 09:08
@csc-felipe
Copy link
Contributor Author

Reverted the exception handling to previous code, and used a simpler approach to reduce complexity.

@csc-felipe csc-felipe merged commit 1b7ea83 into devel Mar 28, 2023
@csc-felipe csc-felipe deleted the feature/pre-commit-2 branch March 28, 2023 13:06
blankdots pushed a commit that referenced this pull request Jun 13, 2023
Fix for tagging functionality in subfolders and files

Closes #1040

See merge request sds-dev/sd-connect/swift-browser-ui!61
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