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

Change Patcher additional_skip_names to accept a list of module #482

Closed
ipwnponies opened this issue May 9, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@ipwnponies
Copy link

commented May 9, 2019

Is your feature request related to a problem? Please describe.
Patcher accepts modules_to_reload as a list of modules to reload. However, additional_skip_names is a set of str. This is mildly inconsistent.

SKIPNAMES = {'os', 'path', 'io', 'genericpath', OS_MODULE, PATH_MODULE}
if pathlib:
SKIPNAMES.add('pathlib')
if pathlib2:
SKIPNAMES.add('pathlib2')
def __init__(self, additional_skip_names=None,

Failure to correctly configure these values results in strange errors deeper down the stack, when the test actually runs. It feels like unnecessary friction and confusion to get the setup just right.

Describe the solution you'd like
How about if additional_skip_names was a list of modules? Internally, the list of modules could be converted to list of str:

self._skip_names = {i.__name__ for i in additional_skip_names}

Could also accept both strings or modules.

@mrbean-bremen

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

Hm, first, this is a feature that is rarely used (out of interest - what do you need it for?).
Secondly, using strings means you don't have to load the modules, so I'm reluctant to change this in general. Your proposal to accept both modules and module names is a good compromise, though.

@ipwnponies

This comment has been minimized.

Copy link
Author

commented May 10, 2019

I'm adding a TTL file cache and am using pendulum to do datetime logic. I use pyfakefs to create the cache in a unit test. pendulum.now() reads both /etc/localtime as well as its packaged tz database files and doesn't work unless allowed to access the real filesystem.

https://github.com/ipwnponies/mealpy/blob/28357c67b3cc82cdd5666fe7b377ffa982a66403/tests/conftest.py#L22-L24

What's the issue with loading the module? Does it interfere with pyfakefs monkey patching and make it more complex to get module reloading right?

@mrbean-bremen

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

No, it should not be a problem - I just don't want to force the user to import it, and want to remain upwards-compatible. Anyway, as I said, I'm changing it to accept both modules and module names .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.