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

Pre-commit update + fix docs build #402

Merged
merged 13 commits into from
Feb 23, 2024
Merged

Pre-commit update + fix docs build #402

merged 13 commits into from
Feb 23, 2024

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Jan 31, 2024

Update of mypy now detects some new issues. Not sure if these are real or a mypy bug, need to investigate more.

EDIT: Looks like mypy was right and we were not ensuring that we return a proper types in one of the functions. The refactor I made is not very pretty but does the trick. Further cleanup would be more work I think.

updates:
- [github.com/pre-commit/pre-commit-hooks: v4.4.0 → v4.5.0](pre-commit/pre-commit-hooks@v4.4.0...v4.5.0)
- [github.com/psf/black: 23.9.1 → 23.12.1](psf/black@23.9.1...23.12.1)
- [github.com/pycqa/isort: 5.12.0 → 5.13.2](PyCQA/isort@5.12.0...5.13.2)
- [github.com/sirosen/check-jsonschema: 0.27.0 → 0.27.3](python-jsonschema/check-jsonschema@0.27.0...0.27.3)
- [github.com/asottile/pyupgrade: v3.13.0 → v3.15.0](asottile/pyupgrade@v3.13.0...v3.15.0)
- [github.com/pre-commit/mirrors-mypy: v1.5.1 → v1.8.0](pre-commit/mirrors-mypy@v1.5.1...v1.8.0)
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 60.09%. Comparing base (d2ea0ec) to head (14d5002).

Files Patch % Lines
aiidalab/app.py 37.50% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
+ Coverage   60.08%   60.09%   +0.01%     
==========================================
  Files          23       23              
  Lines        1458     1466       +8     
==========================================
+ Hits          876      881       +5     
- Misses        582      585       +3     
Flag Coverage Δ
py-3.10 60.09% <37.50%> (+0.01%) ⬆️
py-3.11 60.09% <37.50%> (+0.01%) ⬆️
py-3.8 60.09% <37.50%> (?)
py-3.9 60.09% <37.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz
Copy link
Member

unkcpz commented Feb 1, 2024

For the doc build failure, I guess aiidateam/aiida-core#6253 this will help? Or maybe just simply update the sphinx version to the latest.

@danielhollas danielhollas marked this pull request as ready for review February 23, 2024 02:39
@danielhollas danielhollas changed the title Pre-commit update fix Pre-commit update + fix docs build Feb 23, 2024
@danielhollas
Copy link
Contributor Author

I've managed to fix the docs build in the end (in an ugly way). LMK if you want me to split it to a separate PR.

@yakutovicha
Copy link
Member

I've managed to fix the docs build in the end (in an ugly way). LMK if you want me to split it to a separate PR.

Since fixing Sphinx problems requires adding many dependencies, why don't we switch to a later version of Sphinx itself? Are there any issues with such an update??

@yakutovicha
Copy link
Member

yakutovicha commented Feb 23, 2024

Also, the latest CI test failed because we are still supporting Python 3.8. Time to go on with Python>3.9?

@unkcpz
Copy link
Member

unkcpz commented Feb 23, 2024

Time to go on with Python>3.9?

I think so, we drop it in AWB aiidalab/aiidalab-widgets-base#557

@danielhollas
Copy link
Contributor Author

danielhollas commented Feb 23, 2024

Since fixing Sphinx problems requires adding many dependencies, why don't we switch to a later version of Sphinx itself? Are there any issues with such an update??

Sorry for not being clear, I explain the reason why we can't simply update in a note in setup.cfg
https://github.com/aiidalab/aiidalab/pull/402/files#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52R64

Note that we're not adding new dependencies, we're pinning transitive dependencies that were already there.
IMO the workaround is okay for now, but of course we need to fix it eventually.

Also, the latest CI test failed because we are still supporting Python 3.8. Time to go on with Python>3.9?

This PR has already more stuff than I planned so I'll leave that to the next PR.
For now I simply put pre-commit v3.5 which is still compatible with Python 3.8.

@yakutovicha
Copy link
Member

Since fixing Sphinx problems requires adding many dependencies, why don't we switch to a later version of Sphinx itself? Are there any issues with such an update??

Sorry for not being clear, I explain the reason why we can't simply update in a note in setup.cfg https://github.com/aiidalab/aiidalab/pull/402/files#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52R64

Note that we're not adding new dependencies, we're pinning transitive dependencies that were already there. IMO the workaround is okay for now, but of course we need to fix it eventually.

Also, the latest CI test failed because we are still supporting Python 3.8. Time to go on with Python>3.9?

This PR has already more stuff than I planned so I'll leave that to the next PR. For now I simply put pre-commit v3.5 which is still compatible with Python 3.8.

Thanks for explaining @danielhollas, makes sense to me.

@danielhollas danielhollas merged commit d652291 into main Feb 23, 2024
13 checks passed
@danielhollas danielhollas deleted the pre-commit-update-fix branch February 23, 2024 16:51
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.

None yet

3 participants