aiidateam / disk-objectstore

An implementation of an efficient "object store" (actually, a key-value store) writing files on disk and not requiring a running server
https://disk-objectstore.readthedocs.io
MIT License
16 stars 8 forks source link

Fixing mypy issue renaming database.py to db.py #131

Closed ramirezfranciscof closed 2 years ago

codecov[bot] commented 2 years ago

Codecov Report

Merging #131 (5376b78) into develop (65a3e5d) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #131   +/-   ##
========================================
  Coverage    99.52%   99.52%           
========================================
  Files            8        8           
  Lines         1676     1676           
========================================
  Hits          1668     1668           
  Misses           8        8           
Impacted Files Coverage Δ
disk_objectstore/db.py 100.00% <ø> (ø)
disk_objectstore/container.py 99.39% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 65a3e5d...5376b78. Read the comment docs.

ramirezfranciscof commented 2 years ago

I think this fixes the problems with mypy that we are having on the aiida repository. If I understood correctly, apparently mypy now got more opinionated about files/modules naming.

Example of error in aiidateam/aiida-core: see here.

Example of error in this repo: see here.

The name objectstore_db.py was just to do the test, but if you prefer something else let me know and I'll change it.

sphuber commented 2 years ago

@ramirezfranciscof why do you think the renaming of the module fixed the problem? The issue that you link doesn't seem to explain why database.py should be problematic. They mentioned typing.py, which makes more sense being a problematic name for mypy. I don't see why database.py should cause any problems.

giovannipizzi commented 2 years ago

Indeed, I now checked and while the tests now pass, if we upgrade the version of mypy from 0.910-1 to 0.930 or 0.931, I get again an internal error... So I suggest trying to figure out what actually is the problem (and possibly reverting back this PR).

As a note, the error comes from disk_objectstore/db.py:10 that is a line from sqlalchemy; when not using a recent version of SQLAlchemy with mypy, (e.g. 1.4.13) I was just getting some warnings. Using 1.4.22 that is the fixed version in the pre-commit-config.yaml creates the issue

giovannipizzi commented 2 years ago

Apparently using sqla 1.4.29 fixes the problem. I'm going to revert this PR soon and change the dependency of sqla in the pre-commit-config.yaml