search

erichiggins / gaek

A collection of useful tools for Google App Engine.
MIT License
16 stars 6 forks source link

Fix ndb_json.loads('null') #8

Closed mhluongo closed 8 years ago

mhluongo commented 8 years ago

Use a custom JSONDecoder to avoid bugs decoding top-level nulls, ints, strings, etc, and have better parity with json.loads()

erichiggins commented 8 years ago

Thanks for putting this PR together, @mhluongo ! Would you mind adding a unit test to demonstrate the issue that this change fixes?

mhluongo commented 8 years ago

Sure! Nice response time btw :)

mhluongo commented 8 years ago

It looks like Travis isn't running these tests?

erichiggins commented 8 years ago

Hm, that's strange.

erichiggins commented 8 years ago

At first glance, I don't see an obvious reason for why they aren't being run, do you?

mhluongo commented 8 years ago

Nothing's jumping out at me. When I use nose I typically run that instead of relying on setup.py- might be worth a try

erichiggins commented 8 years ago

Want to try that in this PR?

mhluongo commented 8 years ago

Looks like nose skips executable files.

One of my tests had an issue (apparently NaN != NaN in Python?) but are otherwise good. Looks like there's a legacy test failing as well.

erichiggins commented 8 years ago

Wow, that's weird. Definitely seems to be the root cause though. I'll put together a patch to fix that and my other test so that this PR is unblocked. Thanks again!

erichiggins commented 8 years ago

Ok, I've merged the necessary changes that should unblock you. One thing to note though -- PRs should be merged into the develop branch instead of master. I think I have the wrong default on the repo in GitHub, so I'll need to correct that.

mhluongo commented 8 years ago

@erichiggins do you want this merged to develop or master? Note https://github.com/erichiggins/gaek/pull/9 was merged into master and will need to be merged back into this branch.

erichiggins commented 8 years ago

@mhluongo Yeah, I definitely moved too fast and messed that up. I just merged the changes back into develop so please merge into that branch instead of master. Sorry for the mix-up on my end and confusion.

erichiggins commented 8 years ago

@mhluongo Checking in to see if you'd still like to get this merged. Thanks again for the contribution!

erichiggins commented 8 years ago

I've created #13 to resolve the base-branch issues. Feel free to review if you're still interested.