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

README: mention RemoveCorruptedOutput with an example #11

Merged
merged 7 commits into from Oct 4, 2021
Merged

Conversation

anilbey
Copy link
Collaborator

@anilbey anilbey commented Oct 4, 2021

Updated README to provide an example for the RemoveCorruptedOutputMixin.

@adrien-berchet
Copy link
Member

I think we could also improve the tests of this feature a little bit. For example we could use pytest.raises here: https://github.com/BlueBrain/luigi-tools/blob/main/tests/test_task.py#L851

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
anilbey and others added 2 commits October 4, 2021 14:16
@anilbey
Copy link
Collaborator Author

anilbey commented Oct 4, 2021

I think we could also improve the tests of this feature a little bit. For example we could use pytest.raises here: https://github.com/BlueBrain/luigi-tools/blob/main/tests/test_task.py#L851

Hmm, something fishy is happening with the tests. The tests work as excepted but the Luigi exception is no longer caught.

assert with the complete() method of task instance
@anilbey
Copy link
Collaborator Author

anilbey commented Oct 4, 2021

Ok, the exception gets handled by Luigi. I removed the try blocks and added extra assert statements that check the complete() method of the task instance.

@adrien-berchet
Copy link
Member

Ok, the exception gets handled by Luigi. I removed the try blocks and added extra assert statements that check the complete() method of the task instance.

Ah yes ok. If you want to see an example on how to check the exception raised inside a task you can look here: https://github.com/BlueBrain/luigi-tools/blob/main/tests/test_task.py#L420

But it's not the main point of this feature so it's as you prefer.

@anilbey
Copy link
Collaborator Author

anilbey commented Oct 4, 2021

Ok, the exception gets handled by Luigi. I removed the try blocks and added extra assert statements that check the complete() method of the task instance.

Ah yes ok. If you want to see an example on how to check the exception raised inside a task you can look here: https://github.com/BlueBrain/luigi-tools/blob/main/tests/test_task.py#L420

But it's not the main point of this feature so it's as you prefer.

Thanks!
I added it nonetheless to make sure no other exception but our intended exception is thrown and caught.

Copy link
Member

@adrien-berchet adrien-berchet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@anilbey anilbey merged commit dd70cc4 into main Oct 4, 2021
@anilbey anilbey deleted the readme branch October 4, 2021 15:45
adrien-berchet pushed a commit that referenced this pull request Oct 6, 2021
Add more details for the RemoveCorruptedOutput class in README
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.

None yet

2 participants