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.

Sign up for GitHub

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

Jump to bottom

Added before_reap and after_reap hooks #1104

Merged
merged 2 commits into from
Jun 11, 2019
Merged

Added before_reap and after_reap hooks #1104

merged 2 commits into from
Jun 11, 2019

Conversation

maingoh
Copy link
Contributor

@maingoh maingoh commented Jun 7, 2019 •
edited
Loading

Related to #1103
I am not sure if I need to take care of the hook result ?

@coveralls
Copy link

coveralls commented Jun 7, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.07%) to 62.943% when pulling 8e20c8e on maingoh:reap_hook into b5b485d on circus-tent:master.

@maingoh maingoh mentioned this pull request Jun 10, 2019
Closed
k4nar
k4nar reviewed Jun 10, 2019
View reviewed changes
Copy link
Contributor

@k4nar k4nar left a comment

Choose a reason for hiding this comment

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

This looks good to me! Ideally the doc should be updated, an maybe the tests as well (they can be a bit hard to grasp unfortunately).

@maingoh
Copy link
Contributor Author

maingoh commented Jun 10, 2019

Okay, I will update them. I currently ignore the return value of the hook, should I ? I saw that in the code sometimes it is ignored, sometime not.

@k4nar
Copy link
Contributor

k4nar commented Jun 10, 2019 •
edited
Loading

I think it can be ignored here, yes. We can't do much about it :) .

k4nar
k4nar approved these changes Jun 11, 2019
View reviewed changes
Copy link
Contributor

@k4nar k4nar left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@k4nar
Copy link
Contributor

k4nar commented Jun 11, 2019

The py26 build failed, but I think we should drop it anyway… Let's carry on.

@k4nar k4nar merged commit da45a60 into circus-tent:master Jun 11, 2019
@maingoh
Copy link
Contributor Author

maingoh commented Jun 11, 2019

I had a small question before merging, while testing on my computer, it seems assert* methods where useless. I put a self.assertTrue(False) and the test was still passing. Is it only for me ? I misconfigured something ?
Test should pass however (I checked the kwargs by logging it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k4nar k4nar k4nar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maingoh @coveralls @k4nar