-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Improved test coverage in 5 places for production code #696
Conversation
Pull Request Test Coverage Report for Build 10100505716Details
💛 - Coveralls |
Thanks!
While looking through it: you found mistakes in the code by improving the coverage. 🥳
could you correct that too or create an issue for it?
vCalAddress('b'value'') repr should be vCalAddress('value')
and vBinary repr probably
vBinary(b'value')
My guess is that changing python 2 to 3 did this.
|
Thanks for reviewing my PR. I have corrected them. |
Could you merge master? |
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.
Thanks, I made the changes clearer that I would like to see. Do you think this can be implemented in this pull request?
It is strictly not what the issue said, so I could merge it and create a first-timer issue instead.
What do you prefer?
|
||
def test_repr(): | ||
instance = vBinary("value") | ||
assert repr(instance) == "vBinary('b'value'')" |
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.
assert repr(instance) == "vBinary('b'value'')" | |
assert repr(instance) == "vBinary(b'value')" |
I have fixed the issue. Now you can check. |
@mtr-d3v Thanks! I just merged master so there is no conflict. It seems that the tests are updated and fail. Could you install tox and run the tests in your local setup? I am wondering why not all the tests were running before ... |
I have fixed the repr for vBinary and vCalAddress, the tests should pass now. |
Thank you very much! |
This PR fixes issue #629 by Improving test coverage in 5 places for production code
📚 Documentation preview 📚: https://icalendar--696.org.readthedocs.build/