the-siesta-group / edfio

Read and write EDF/EDF+ files.
Apache License 2.0
25 stars 5 forks source link

BUG: Fix bug with version #46

Closed larsoner closed 3 months ago

larsoner commented 3 months ago

On latest main I just did:

$ git clone git@github.com:/the-siesta-group/edfio.git
$ cd edfio
$ pip install -ve .
...
Successfully built edfio
Installing collected packages: edfio
Successfully installed edfio-0.4.0.post28+b17cad2
$ cd ~
$ python -c "import edfio; print(edfio)"
<module 'edfio' from '/home/larsoner/python/edfio/edfio/__init__.py'>
$ python -c "import edfio; print(edfio.__version__)"
0.0.0

So I don't think this is yet working as intended even with #44. After the changes in this PR (which follow what MNE-Python does to get the version) I get:

$ python -c "import edfio; print(edfio.__version__)"
0.4.0.post28+b17cad2
hofaflo commented 3 months ago

Right, thanks for the fix! In which case(s) could we expect the code in the try-block to raise an exception?

larsoner commented 3 months ago

No idea I think it's just a safety thing. import edfio failing because some metadata is messed up in .dist-info (or whatever other errors the version inspection might trigger!) seems like less good behavior than import edfio working but the version being reported as 0.0.0

hofaflo commented 3 months ago

Makes sense, it e.g. fails if the package is just imported locally but not installed. To make the test job happy, could you please add a # pragma: no cover to the except Exception: line and replace exclude_lines with exclude_also in pyproject.toml?

larsoner commented 3 months ago

Done in https://github.com/the-siesta-group/edfio/pull/46/commits/313a24de29225ce1064d23595e01c382784a2782 (not 100% sure if you wanted me to add anything to exclude_also or not -- wasn't sure if the pragma was enough)

hofaflo commented 3 months ago

Perfect as-is, thanks @larsoner!