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

Upgrade to python3.13 and other dependencies #133

Conversation

lumpy72006
Copy link
Contributor

@lumpy72006 lumpy72006 commented Feb 8, 2025

fix #132
Ready for review

@benoit74
Copy link
Contributor

benoit74 commented Feb 9, 2025

  • is this ready for review? (please inform us everytime PR is ready for review, or it is impossible for us to know if it is still ongoing work or complete)
  • why did you had to add pytest to check dependencies? Unless I'm mistaken, it is not supposed to be used by check tasks and is already part of the test dependencies?
  • please add Fix #xxx text in first comment to link PR with issue (where xxx is fixed issue number ; multiple issues might be fixed by one PR): https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

@lumpy72006
Copy link
Contributor Author

  • is this ready for review? (please inform us everytime PR is ready for review, or it is impossible for us to know if it is still ongoing work or complete)
  • why did you had to add pytest to check dependencies? Unless I'm mistaken, it is not supposed to be used by check tasks and is already part of the test dependencies?
  • please add Fix #xxx text in first comment to link PR with issue (where xxx is fixed issue number ; multiple issues might be fixed by one PR): https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

I had to add it because when I ran hatch run check:all, I got a total of 205 errors and this /home/lumpy/mindtouch/scraper/tests/test_asset.py:1:8 - error: Import "pytest" could not be resolved (reportMissingImports) was the first. I thought maybe check couldn't access pytest and when I added it I got 0 errors.

@lumpy72006 lumpy72006 closed this Feb 9, 2025
@lumpy72006 lumpy72006 reopened this Feb 9, 2025
@lumpy72006
Copy link
Contributor Author

I fixed it now. Sorry if I'm stressing you 🙏

@lumpy72006 lumpy72006 force-pushed the upgrade-to-python3.13-and-other-dependencies branch from 560f64b to bd159ed Compare February 9, 2025 16:36
@benoit74 benoit74 self-requested a review February 10, 2025 08:53
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.92%. Comparing base (097f298) to head (bd159ed).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #133   +/-   ##
=======================================
  Coverage   50.92%   50.92%           
=======================================
  Files          20       20           
  Lines        1186     1186           
  Branches      137      137           
=======================================
  Hits          604      604           
  Misses        569      569           
  Partials       13       13           

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

Copy link
Contributor

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

See inline comments for small changes + CI is failing, the Dockerfile base image needs to be updated to use Python 3.13 as well (it wasn't specified in the issue tbh).

lint = ["black==24.10.0", "ruff==0.9.1"]
check = ["pyright==1.1.391"]
lint = ["black==25.1.0", "ruff==0.9.5"]
check = ["pyright==1.1.393"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since pytest is needed for check operations to run, then please add it back here (my question is only a question to understand things as fast as possible avoiding to have to explore things you've already understood). Or even better, add mindtouch2zim[test] to avoid repeating the pytest version.

build-backend = "hatchling.build"

[project]
name = "mindtouch2zim"
requires-python = ">=3.12,<3.13"
requires-python = ">=3.13,<3.14" # require python 3.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this comment, it should be obvious to most developpers

description = "Make ZIM file from Mindtouch / Nice CXone Expert libraries"
readme = "../README.md"
dependencies = [
"zimscraperlib==5.0.0",
"zimscraperlib==5.1.0", # upgrade to 5.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this comment, it means nothing outside this PR

@lumpy72006 lumpy72006 force-pushed the upgrade-to-python3.13-and-other-dependencies branch from bd159ed to df79423 Compare February 10, 2025 12:39
@lumpy72006 lumpy72006 closed this Feb 10, 2025
@lumpy72006 lumpy72006 force-pushed the upgrade-to-python3.13-and-other-dependencies branch from df79423 to 097f298 Compare February 10, 2025 12:54
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.

Upgrade to Python 3.13 and other dependencies
2 participants