-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add: get_all_cases
extended to support filtering and use other modules as parametrization_target
#260
Add: get_all_cases
extended to support filtering and use other modules as parametrization_target
#260
Conversation
get_all_cases
doesn't require parametrization targetget_all_cases
without parametrization target
@smarie As described in the PR body, I don't have easy access to test all of these with my usual Python workflow but the version I could test seemed fine. Let me know if anything need changing! |
if parametrization_target is not None: | ||
caller_module_name = getattr(parametrization_target, '__module__', None) | ||
parent_pkg_name = '.'.join(caller_module_name.split('.')[:-1]) if caller_module_name is not None else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be merged with the above branch of the if ?
Also, if parametrization_target
is not used at all below this point and only caller_module_name
/ parent_pkg_name
are, we could imagine to accept both
- None (new default as per your proposal)
- a module name (in case you wish to pre-collect various cases from an entire module, in order to resolve the relative imports and AUTO mode). Note: I may be too optimistic here, maybe a module name is not enough. An alternative is to accept a module object
- a parametrization target
Finally, when None is passed, we could rely on the get_caller_module
utility function in order to still grab the calling module (from .fixture__creation import get_caller_module
). This should only be done if actually needed (son, if a relative path or AUTO mode is used)
Does it make sense ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it could be merged, just made into an else
statement. I was thinking to put it into the for loop but it made more sense to catch the error early to me.
I'm not sure I understand the bullet points but given the last statement of using get_caller_module
, how does this behaviour sound
# The current behaviour
def get_all_cases(f, cases=...)
# The functionality implemeneted
def get_all_cases(cases=[case_1, ..., case_n])
# Change this from being an error to being valid.
# Would use `get_caller_module` so these args work as expected
def get_all_cases(cases='.')
def get_all_cases(cases=AUTO)
def get_all_cases(cases='.xyz')
With respect to:
a module name (in case you wish to pre-collect various cases from an entire module, in order to resolve the relative imports and AUTO mode). Note: I may be too optimistic here, maybe a module name is not enough. An alternative is to accept a module object
Would this functionality be solved with the cases='.'
or cases=AUTO
way of calling as illustrated in the last section of the code snippet above? If not, could you provide a code snippet of what args you would pass in and what the behaviour would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Change this from being an error to being valid.
def get_all_cases(cases='.')
def get_all_cases(cases=AUTO)
def get_all_cases(cases='.xyz')
Would use get_caller_module
so these args work as expected: YES
We would alternately accept an explicit module object (or name ?) to explicitly help resolve the caller module
import test_module
get_all_cases(test_module, cases=AUTO)
I am just thinking that if we only need a module to perform this action, then we should not need users to pass a dummy function. So either the module is implicit (from the call stack, get_caller_module
), or it is explicit and therefore we would accept that parametrization_target
(or a new param) receives it.
Maybe this can be done in a separate PR if it sounds too cumbersome. For now you can stick to the use case you had in mind (implicit or none).
I updated to implement the functionality described in the review comment so you can see and test the behaviour. I would still need to modify docs and do any other changes :) |
Hi @smarie, I implement the behaviour described here: I've also tested it locally but only with Python versions I had installed. Hopefully this will pass the checks anywho. Let me know if there's anything else :) Best, |
get_all_cases
without parametrization targetget_all_cases
extended to support filtering and use other modules as parametrization_target
THanks @eddiebergman ! and very sorry for the long review delay. I'll have a look |
Hi @smarie, No worries, there is no rush for usage, just a nice feature to use for the future :) Take your time Best, |
Hi @smarie, Is there anything else you would need me to do for these PR's? I'm rewriting tests for our library autosklearn and this would be a handy feature :) |
@@ -635,31 +646,41 @@ def _get_fixture_cases(module_or_class # type: Union[ModuleType, Type] | |||
return cache, imported_fixtures_list | |||
|
|||
|
|||
def import_default_cases_module(f): | |||
def import_default_cases_module(context): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If context
is not actually used anywhere in this function except as a way to read the module __name__
, then I suggest to replace it with an explicit test_module_name
argument.
Sorry @eddiebergman for this delay ; The reason why I did not validate at first glance a few days ago is that I thought "maybe this is the time to make the signature of However I was not able to come up with a nicer signature, so I just suggest for now one mod (see comments) to prepare the ground, and I'll open a distinct issue once this is merged, about "finding a better signature". If you can find some time to do the tiny mod above, then I guess we can merge and release fast. |
I don't have any better suggestion as a Perhaps the ideal solution would be to split out the functionality, i.e. introduce a specific function I'll do the modification as asked and ping you once it's done :) |
I eventually merged and did the mod myself @eddiebergman , as I wanted to do a release today. Thanks ! |
Ref #258
For collecting cases without specifying a target, this PR extends
get_all_cases
to optionally take aparametrization_target
to allow for filtering cases which are explicitly given usingget_all_cases(cases=[case_1, case_2, ...])
.This PR also enabled direct module references to be given instead of just function references for
parametrization_target
to increase its flexibility when using relativecases
such ascases='.'
,cases=AUTO
. This enables things likeRetrieve cases for test modules
Filtering
Note
I did not modify
get_paramtrize_args
to extend the functionality there as it was out of scope and the chain of the parameter for the module gets passed through quite a few functions.