-
Notifications
You must be signed in to change notification settings - Fork 97
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
Conversation
The changes look fine. does it require changing the github action as well since actions are failing?
My understanding was that pytype catches more bugs by static analysis. Has mypy improved? link: https://x.com/iavins/status/1524407933982765056 |
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 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. |
This should fix the tests and mypy, hopefully. |
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. |
Hey, pytype is updated and I am sure it works. Can you try with the latest version? I wouldn't remove it just yet |
sure let me try it. |
This reverts commit 43ee33e.
I see what happened. |
Thank you @tusharsadhwani 🥳 |
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 offrom caskdb
importRemove
self.assertTrue()
from a test, which was causing mypy to fail inmemory_store.py
Move
example.py
inside the package, so you can dopython -m caskdb.example
to run it.Add a
setup.cfg
and dummysetup.py
to make the package installable, moved dev dependencies tosetup.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 runningpip install -e .[dev]
as in:.
(current directory),-e
) package,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 ignorevenv
andsetup.py
from raising typecheck issuesAdd
build
as the install tool, andtwine
as the tool to upload the wheels.To test these changes:
Create a venv
python3 -m venv venv
and activate itsource venv/bin/activate
Run
pip install -r requirements_dev.txt
Run
pytest
to ensure that we are indeed running tests on the packaged version ofcaskdb
. The tests will no longer pass without runningpip install
on our package now, becauseformat
,disk_store
etc. are no longer import-able directly without thecaskdb.
prefix.This is also why we chose the
src/
layout, because if we just put them in acaskdb/
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 insetup.cfg
to1.0.0
if you wish.Resolves #5