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

Add tests for task #11

Merged
merged 6 commits into from
Apr 27, 2024
Merged

Conversation

nathanjmcdougall
Copy link
Contributor

Lots of unit tests.

Two are failing. _lt is a reserved keyword, but it is also being used as a way to detect classes which are tasks - and we don't raise errors about reserved attributes in such cases. Is this just for if we are inheriting from a previously defined Task class? I wonder whether there is a better way to do this (perhaps by inspecting directly the MRO), because I think we still want to protect the user if they accidentally use _lt as an attribute.

Keen to know what you'd do about this.

@nathanjmcdougall nathanjmcdougall marked this pull request as draft April 10, 2024 05:54
@nathanjmcdougall
Copy link
Contributor Author

Need to pass the linter

@nathanjmcdougall nathanjmcdougall marked this pull request as ready for review April 10, 2024 11:33
@ben-denham
Copy link
Owner

These tests look great thanks, and good catch with _lt. You're correct that the is_task_type() guard around attribute checking is for handling inheritance (that probably deserves a comment). I think we should handle this by making is_task_type() more robust (as well as is_task() while we're at it):

def is_task_type(cls):
    """Returns `True` if the given `cls` is a class decorated with
    [`labtech.task`][labtech.task]."""
    return isclass(cls) and isinstance(getattr(cls, '_lt', None), TaskInfo)


def is_task(obj):
    """Returns `True` if the given `obj` is an instance of a task class."""
    return is_task_type(type(obj)) and hasattr(obj, '_is_task')

@ben-denham ben-denham merged commit f61942d into ben-denham:main Apr 27, 2024
3 checks passed
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.

2 participants