Coerce empty string to nil for all Primitive types except String

[petekinnecom]

No description provided.

[petekinnecom]

Here's a reproduction of the behavior on v.1.2.5: petekinnecom@ec99d8c#diff-d4c132540f49911bbd1c73264c2c738bR12

[dnesteryuk]

It looks right to me 👍

[dblock]

This does modify external behavior of interacting with an API. I think we need:

• A spec as comprehensive as petekinnecom@ec99d8c#diff-d4c132540f49911bbd1c73264c2c738bR12
• A API-level spec describing the behavior in Date coercion discrepancy between 1.2.5 and 1.3.0 #2066

WDYT?

[dnesteryuk]

@dblock do you mean more integration tests? What is wrong with unit tests? 🤔 Personally, I like more unit tests, they are more focused (point to code under tests).

[dblock]

@dblock do you mean more integration tests? What is wrong with unit tests? 🤔 Personally, I like more unit tests, they are more focused (point to code under tests).

Yes. This behavior affects what happens when you call an API, and there are no changes in tests for that external behavior. We often rely on those integration tests when we're not sure what's supposed to happen and still keep introducing regressions. Better safe than sorry. @petekinnecom if you aren't adding any I'm OK with merging this though. Just let us know?

[petekinnecom]

I've pushed a commit that has integration tests that mirror the tests found for nil (they are found just above in the same file I edited). The behavior for primitive types is all that's been updated here, so for other types, this just captures existing behavior. For the exceptions here the params are not parsed as an empty string but rather cause a 400 response:

619db09#diff-d4c132540f49911bbd1c73264c2c738bR533
619db09#diff-d4c132540f49911bbd1c73264c2c738bR550

[dblock]

Perfect, merged. Thank you.