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

Package caskdb for PyPI #19

Merged
merged 14 commits into from
Jul 5, 2024
Merged

Package caskdb for PyPI #19

merged 14 commits into from
Jul 5, 2024

Conversation

tusharsadhwani
Copy link
Contributor

@tusharsadhwani tusharsadhwani commented Jun 27, 2024

Summary

  • Move the code to a src/caskdb/ folder, for packaging and testing convenience (will come back to this later)

  • Change the module imports to be from caskdb.disk_store import ...

  • Added __init__.py with __all__ to specify the public API of from caskdb import

  • Remove self.assertTrue() from a test, which was causing mypy to fail in memory_store.py

  • Move example.py inside the package, so you can do python -m caskdb.example to run it.

  • Add a setup.cfg and dummy setup.py to make the package installable, moved dev dependencies to setup.cfg.
    A pyproject.toml would work too but it's the exact same setup in a bit more verbose toml format.

  • Now requirements_dev.txt simply contains -e .[dev], which is equivalent to running pip install -e .[dev] as in:

    • install . (current directory),
    • as an editable (-e) package,
    • with dev dependencies included.

    Editable installs mean when you change the code, the package reflects the change without needing to reinstall it.

  • Added a mypy.ini file to ignore venv and setup.py from raising typecheck issues

  • Add build as the install tool, and twine as the tool to upload the wheels.

To test these changes:

  • Create a venv python3 -m venv venv and activate it source venv/bin/activate

    personally I recommend putting export PIP_REQUIRE_VIRTUALENV=false in your bashrc file to avoid accidentally pip installing stuff in your system Python.

  • Run pip install -r requirements_dev.txt

  • Run pytest to ensure that we are indeed running tests on the packaged version of caskdb. The tests will no longer pass without running pip install on our package now, because format, disk_store etc. are no longer import-able directly without the caskdb. prefix.

    This is also why we chose the src/ layout, because if we just put them in a caskdb/ folder, they would still be importable by the tests directly. That way the tests passing won't tell you if the package is correctly being installed or not.

  • If tests pass, build the package by doing python -m build

  • The upload the packages by creating an API key on PyPI, then running twine upload ./dist/*

Once this is all done, you can pip install caskdb to install the package.


Other than this, I think pytype is redundant in the dependencies, mypy does the job fine, and also you might want to change the version of caskdb in setup.cfg to 1.0.0 if you wish.

Resolves #5

@avinassh
Copy link
Owner

avinassh commented Jul 3, 2024

The changes look fine. does it require changing the github action as well since actions are failing?

Other than this, I think pytype is redundant in the dependencies, mypy does the job fine

My understanding was that pytype catches more bugs by static analysis. Has mypy improved? link: https://x.com/iavins/status/1524407933982765056

@tusharsadhwani
Copy link
Contributor Author

tusharsadhwani commented Jul 3, 2024

My understanding was that pytype catches more bugs by static analysis

Both mypy and pytype implement the same exact tech, it's just that mypy chooses not to type check inside functions whose arguments are untyped (That's why that case wasn't caught by mypy, the concat function is untyped). This allows mypy to be friendly with semi-typed codebases, or codebases that are opting into typing one module at a time.

Generally if you're going to type annotate all your code, the output of both should be identical. But yes, if the codebase is not entirely type annotated, YMMV.

@tusharsadhwani
Copy link
Contributor Author

This should fix the tests and mypy, hopefully.

@tusharsadhwani
Copy link
Contributor Author

Seems like pytype doesn't work on Python 3.11+ as in it hasn't added new Python support for over 2 years now. I think it's best to remove it for the future.

@avinassh
Copy link
Owner

avinassh commented Jul 4, 2024

Hey, pytype is updated and I am sure it works. Can you try with the latest version? I wouldn't remove it just yet

@tusharsadhwani
Copy link
Contributor Author

sure let me try it.

@avinassh
Copy link
Owner

avinassh commented Jul 4, 2024

ref: google/pytype#1308 (comment)

@tusharsadhwani
Copy link
Contributor Author

I see what happened. pytype uses calendar versioning, so it updated to the 2023 and 2024 versions of the package.

@avinassh avinassh merged commit e8162bf into avinassh:master Jul 5, 2024
6 of 8 checks passed
@avinassh
Copy link
Owner

avinassh commented Jul 5, 2024

Thank you @tusharsadhwani 🥳

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.

2 participants