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

Increase test coverage for Event class by 10% #346

Merged
merged 3 commits into from
Jul 15, 2022

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Jun 19, 2022

Improve test coverage for event.py, getting closer to 90% coverage.
Coverage improvements include duration, gt/lt comparisons, within, includes, and intersects methods.

Issue #245

@C4ptainCrunch C4ptainCrunch added the merge-before-black We should merge this PR before running Black label Jul 4, 2022
@allenporter
Copy link
Contributor Author

Thanks for the review. Is there a path to merging? I'm trying to decide if it's worthwhile to continue to contribute here. I get the point of merging before applying black but unsure what we're waiting on.

@C4ptainCrunch
Copy link
Member

@allenporter To me, this is good to merge, i can merge it when you want. But i can also wait if you want to work some more on it :)

@allenporter
Copy link
Contributor Author

@allenporter To me, this is good to merge, i can merge it when you want. But i can also wait if you want to work some more on it :)

Oh no, to clarify, i'm not saying i'm trying to add more to this PR. It was finished 22 days ago and i'm not sure what the next steps are. I was planning to add a huge number of tests, but no I'm trying to decide if it's worthwhile to contribute here at all.

@C4ptainCrunch C4ptainCrunch merged commit b1996f4 into ics-py:main Jul 15, 2022
@C4ptainCrunch
Copy link
Member

I'm trying to decide if it's worthwhile to contribute here at all.

@allenporter You work is appreciated, you can be sure of it ! If you want to write more tests, it will be welcome ❤️

@strobeflash
Copy link
Contributor

Oh no, to clarify, i'm not saying i'm trying to add more to this PR. It was finished 22 days ago and i'm not sure what the next steps are. I was planning to add a huge number of tests, but no I'm trying to decide if it's worthwhile to contribute here at all.

@allenporter Thank you for your contributions! Don't leave me, between your tests and my docs we could achieve great things! :) When could you have your next pull request ready? @C4ptainCrunch, when do you think you'll be able to make time to review? I'll try to have something ready too by then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-before-black We should merge this PR before running Black
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants