fix: Treat None as UNSET in query params for model properties, etc.

[forest-benchling]

Fixes some missing cases not covered by the original implementation in #331. In particular, properties with non-trivial serialization would crash when None was passed instead of UNSET.

Closes #380.

[codecov]

Codecov Report

Merging #421 (f7f6b9e) into main (032a4a4) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #421   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1549      1549           
=========================================
  Hits          1549      1549           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 032a4a4...f7f6b9e. Read the comment docs.

[forest-benchling]

@dbanty @emann (cc @dtkav @GitOnUp)

[dbanty]

I'm not sure I totally follow here. Is the goal to account for when someone disregards the type annotation and passes None to something that is not nullable? I don't think we want excess runtime type checking just in case someone ignores the types. One of the goals of this project is to not have extraneous runtime costs everywhere because we can do static checking.

The checks at the end already strip out any nullable params which are None, so I'm not sure what else this PR is doing. Am I missing something?

[forest-benchling]

I'm not sure I totally follow here. Is the goal to account for when someone disregards the type annotation and passes None to something that is not nullable? I don't think we want excess runtime type checking just in case someone ignores the types. One of the goals of this project is to not have extraneous runtime costs everywhere because we can do static checking.

The checks at the end already strip out any nullable params which are None, so I'm not sure what else this PR is doing. Am I missing something?

@dbanty Yeah, I agree this is confusing. However, it's what we had decided to do in #285. Remember that I had initially implemented a solution that would change the type of non-nullable optional properties to allow None, but we decided to abandon it because it was too complex? 😅

I also forgot to link the issue closed here, which is #380.

[codecov-commenter]

Codecov Report

Merging #421 (4be4790) into main (605c246) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #421   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1563      1563           
=========================================
  Hits          1563      1563           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 605c246...4be4790. Read the comment docs.

[dbanty]

@forest-benchling I've opened #462 to replace this. It takes a slightly different strategy of just always treating nullable and not required as both of those things. That way the type annotations still match what we're expecting folks to pass in. Please review that when you get a chance to verify that it suits Benchling's needs.