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

admin/fix-ci-and-upgrade-lint-dependencies #6

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

SeanLeRoy
Copy link
Contributor

Description of Changes

The CI was failing due to an issue with isort and poetry, upgrading isort to >5.12.0 fixes this issue. Beyond that I noticed the versions were specified in both .pre-commit-config.yaml & pyproject.toml, it doesn't appear that pre-commit requires any of these dependencies to be installed prior to running pre-commit based on it ignoring whatever version I specify in pyproject.toml and solely using the version specified by the rev in the pre-commit-config.yaml. I tested this on MacOS as described below, but if I'm missing the value of pyproject.toml having the versions too let me know!

Testing

Created a fresh virtual environment, installed dependencies, and ran just lint & just test

@SeanLeRoy SeanLeRoy self-assigned this Jun 27, 2023
@@ -41,14 +41,6 @@ Documentation = "https://bioio-devs.github.io/bioio"
# https://peps.python.org/pep-0621/#dependencies-optional-dependencies
[project.optional-dependencies]
lint = [
"black>=22.3.0",
"check-manifest>=0.48",
Copy link
Contributor Author

@SeanLeRoy SeanLeRoy Jun 27, 2023

Choose a reason for hiding this comment

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

check-manifest wasn't being used in pre-commit like the others, instead it was installed without a specific version before just install was ran here so it seems safe to delete unless I am missing a usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason I include black is that VSCode will auto format on save when it is installed.

(I may be wrong about that, that is how I have set it up anyway)

Check manifest can go. It's very useful but used infrequently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, does VSCode not need any of these installed to have all the nice linting, formatting, type hinting, etc?

@SeanLeRoy SeanLeRoy marked this pull request as ready for review June 27, 2023 16:19
@SeanLeRoy SeanLeRoy requested a review from BrianWhitneyAI June 27, 2023 16:19
@SeanLeRoy SeanLeRoy changed the title admim/fix-ci-and-upgrade-lint-dependencies admin/fix-ci-and-upgrade-lint-dependencies Jun 27, 2023
@SeanLeRoy SeanLeRoy merged commit 70804df into main Jun 28, 2023
@SeanLeRoy SeanLeRoy deleted the admin/fix-ci-and-upgrade-linting branch June 28, 2023 18:50
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.

4 participants