cldf-clts / clts

Cross-Linguistic Transcription Systems
https://clts.clld.org
14 stars 3 forks source link

pkg_path not in pyclts.util #28

Closed Anaphory closed 4 years ago

Anaphory commented 4 years ago

I just ran pytest --doctest-modules on a piece of code that somewhere imports pyclts. I'm not entirely sure why that leads down the rabbit hole of checking the sanity of some clts modules, but it does:

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
clts/sources/apics/scraper.py:3: in <module>
    from pyclts.util import pkg_path
E   ImportError: cannot import name 'pkg_path' from 'pyclts.util' (/home/gereon/.local/etc/lexedata/lib/python3.8/site-packages/pyclts/util.py)

This is true, there is that import there and also in some of the other scrapers:

https://github.com/cldf-clts/clts/blob/d0dbd4bd5c1c0745db15d113745c685cdd95e9d9/sources/apics/scraper.py#L3

and the pyclts package does not seem to contain the string pkg_path anywhere at all, neither in the current version on GitHub nor in the version on pypi.

xrotwang commented 4 years ago

Isn't that rather a problem of '--doctest-modules' which seems to try and import everything?

Gereon Kaiping notifications@github.com schrieb am So., 2. Aug. 2020, 21:03:

I just ran pytest --doctest-modules on a piece of code that somewhere imports pyclts. I'm not entirely sure why that leads down the rabbit hole of checking the sanity of some clts modules, but it does:

traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clts/sources/apics/scraper.py:3: in from pyclts.util import pkg_path E ImportError: cannot import name 'pkg_path' from 'pyclts.util' (/home/gereon/.local/etc/lexedata/lib/python3.8/site-packages/pyclts/util.py)

This is true, there is that import there and also in some of the other scrapers:

https://github.com/cldf-clts/clts/blob/d0dbd4bd5c1c0745db15d113745c685cdd95e9d9/sources/apics/scraper.py#L3

and the pyclts package does not seem to contain the string pkg_path anywhere at all, neither in the current version on GitHub nor in the version on pypi.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cldf-clts/clts/issues/28, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUOKHXKM2MNEBBZ6R3U7DR6W2BLANCNFSM4PSV4DYQ .

xrotwang commented 4 years ago

But yes, non-functioning code shouldn't be part of the package.

Robert Forkel xrotwang@googlemail.com schrieb am So., 2. Aug. 2020, 21:33:

Isn't that rather a problem of '--doctest-modules' which seems to try and import everything?

Gereon Kaiping notifications@github.com schrieb am So., 2. Aug. 2020, 21:03:

I just ran pytest --doctest-modules on a piece of code that somewhere imports pyclts. I'm not entirely sure why that leads down the rabbit hole of checking the sanity of some clts modules, but it does:

traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> clts/sources/apics/scraper.py:3: in from pyclts.util import pkg_path E ImportError: cannot import name 'pkg_path' from 'pyclts.util' (/home/gereon/.local/etc/lexedata/lib/python3.8/site-packages/pyclts/util.py)

This is true, there is that import there and also in some of the other scrapers:

https://github.com/cldf-clts/clts/blob/d0dbd4bd5c1c0745db15d113745c685cdd95e9d9/sources/apics/scraper.py#L3

and the pyclts package does not seem to contain the string pkg_path anywhere at all, neither in the current version on GitHub nor in the version on pypi.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cldf-clts/clts/issues/28, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUOKHXKM2MNEBBZ6R3U7DR6W2BLANCNFSM4PSV4DYQ .

Anaphory commented 4 years ago

Oh, the reason was that I had a stray clone of this repository in my code base from a mistake with cldfbench, so not pytest's fault. If there hadn't been strange code of cldf exposed due to that mistake, I would not have started it as an issue here!

xrotwang commented 4 years ago

Ok, just looked at this issue. These legacy modules are not even in pyclts, but in clts/sources. Neither sources nor the individual directories are marked as python packages (i.e. don't have a __init__.py). So even if clts/sources was in your python path, it shouldn't be possible to inadvertently import this legacy code. Thus, I still think --doctest-modules is overly ambitious - or should never be used without narrowing the test directories. Making sure all versions of datasets contain only code that is compatible (in terms of imports) with all versions of packages is certainly out of scope.

That said, I agree that we should remove obsolete code wherever it is - or at least annotate it with compatible package versions.