search

closeio / cleancat

Validation and serialization library for Python designed to be used with JSON REST frameworks
MIT License
52 stars 8 forks source link

Clean up MongoEmbeddedReference #25

Closed wojcikstefan closed 6 years ago

wojcikstefan commented 6 years ago

Commits can be reviewed separately. The first commit introduces no logical changes. The second one fixes a redundant call to Dict.clean (I've confirmed that it was called twice for a single POST/PUT in our API).

wojcikstefan commented 6 years ago 
+        # Dict. Dict's validation is performed before the clean() method is
+        # even called.

How so? The only reason you can skip Dict.clean() is because you already have a value so the required check is not needed.

Ah, you're right, got confused there for a second. Now that I look at the code flow, we should still call Dict.clean, though I'd argue it should be the first thing we call in MongoEmbeddedReference.clean, agreed?

it doesn't appear to fix any issues.

Indeed, this is just an attempt at making all of this more readable.

thomasst commented 6 years ago

should be the first thing we call in MongoEmbeddedReference.clean

That makes sense, but I'm not sure why I didn't do it that way in the first place, so make sure to test this well.

Also any chance you can move all the mongo fields out of base.py? :)

wojcikstefan commented 6 years ago

That makes sense, but I'm not sure why I didn't do it that way in the first place, so make sure to test this well.

I ran our lead & contact tests (which are pretty extensive) against this branch and it works well (including the recently added test: https://github.com/closeio/closeio/commit/627b4e730b1926daf45bfd9636ab06d445918f12).

Will split out mongo-specific stuff in a follow-up PR (already have the code for it). Let's merge what we already have here first.