search

stefankoegl / python-json-pointer

Resolve JSON Pointers in Python
https://python-json-pointer.readthedocs.org/
Other
141 stars 43 forks source link

Ensuring the return of the default and some small refactoring #10

Closed thekafkaf closed 9 years ago

thekafkaf commented 9 years ago

Catching a broad except to ensure the default return(#9), added tox, fixed some py3 compatibility issues, added some tests, cleaned up a bit(flake8), small refactoring

thekafkaf commented 9 years ago

Hi, first of all the fix I was 'assigned' to was very small, and doesn't require this whole pr. Now, I think, catching a broad except is a must - to ensure the behavior of returning the default. I don't think we need to have a specific handling for the cases demonstrated in the issue. I did some refactoring, which is quite rude, and I accept that, just tell me, and I'll quickly reverse it. Thank you for your patince.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same when pulling a61e0c06871141339c8cc99f9d143c5fd30aac71 on thekafkaf:bugfix/unexpected-errors-on-resolving into 490c7a25147b0674fc0234d0462726595bd4fd87 on stefankoegl:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same when pulling a61e0c06871141339c8cc99f9d143c5fd30aac71 on thekafkaf:bugfix/unexpected-errors-on-resolving into 490c7a25147b0674fc0234d0462726595bd4fd87 on stefankoegl:master.

stefankoegl commented 9 years ago

Thanks for your pull request. I do not consider your refactoring as rude, but the PR as it is, is not really clear to me. It combines too many canges (refactoring, tests using tox, the exception handling, whitespace changes) into one.

I'd suggest to re-submit these individual changes as separate PR. Please let me know if that's ok.

thekafkaf commented 9 years ago

Ok no problem, I'll split it to two pr's one for the refactoring and one for the exception handling, as the use of tox is just adding the tox.ini, and now you can run your tests on (multiple) virtualenvs.