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

TST, API: test case where edfapi C library not installed #12

Merged
merged 8 commits into from
Dec 27, 2024

Conversation

scott-huberty
Copy link
Owner

@scott-huberty scott-huberty commented Nov 13, 2024

Follow up from #11 - I added a unit test with a monkey patch to test the case where the SR Research edfapi C library is not installed on the users machine. The test checks that our informative error message is thrown as expected.

As far as I could work it out, this required a change in one of the module names.. read_edf.py -> read.py , because there is also a function named read_edf that lived in read_edf.py, and gets directly imported to the edf namespace by eyelinkio.edf.__init__.py, causing the read_edf function to overshadow the module of the same name.... e.g.

# on main branch 11/13/2024
In [1]: import eyelinkio

# One might be trying to access the read_edf module but are getting the function...
In [2]: eyelinkio.edf.read_edf
Out[2]: <function eyelinkio.edf.read_edf.read_edf(fname)>

So the unittest.mock.patch I added in this PR ended up trying to patch the function, not the module, causing an error.

And I do think it's better to differentiate the module namespace from the function anyways.

The documented public API was and still is from eyelinkio import read_edf (which imports the function) so this change hopefully will not break anyones code.


EDIT: An alternative approach would be to avoid importing the read_edf function into the same namespace as the read_edf module:

https://github.com/scott-huberty/eyelinkio/blob/7f5e2e5ea819afbd6b761aa2b844c78010d8d805/src/eyelinkio/edf/__init__.py#L2C1-L2C36

But I still prefer the approach I've taken, because I think that it makes it easier to distinguish between the module and the function (now the module is read.py and the function is read_edf)

This is so that we don't confused the read_edf.py module with the read_edf function that
is part of the read_edf.py module.

The documented public API was and remains `from eyelinkio import read_edf` , which imports the function,
so this should not have any compat issues with existing user code, unless they explicitly are doing something
like eyelinkio.edf.read_edf.some_function
@scott-huberty scott-huberty changed the title TST, API: add test for TST, API: test case where edfapi C library not installed Nov 13, 2024
…lled

I had to resolve the new directory structure on main ( src/eyelinkio ) with my renaming of modules in this PR (i.e. eyelinkio/read.py )
@scott-huberty scott-huberty merged commit c3afb9a into main Dec 27, 2024
9 checks passed
@scott-huberty scott-huberty deleted the test_edfapi_not_installed branch December 27, 2024 21:05
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.

1 participant