Skip to content

Conversation

@dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented May 16, 2020

A while back it has been suggested to make the loader helpers check for zeroes implicitly, raising an error. While that seems good to do initially, it leads to a situation where users will check for zeroes manually to avoid throwing, and the helper will check for zero once again in case it has to throw. To avoid redundant checks like these, this PR instead documents the behavior more clearly, expecting users to check for zeroes where necessary. In the worst case there'll be one check in user code this way, while in the best case there's no check at all.

See: #381

@dcodeIO dcodeIO merged commit 088eaad into master May 16, 2020
@jtenner
Copy link
Contributor

jtenner commented May 16, 2020

As-pect wraps the getString function, checking for 0 values, and returning a default value instead of the string when its 0.

Would it make sense to let getString return null instead?

@dcodeIO
Copy link
Member Author

dcodeIO commented May 16, 2020

This is similar in nature, in that the helper will always have to include a check for 0 even if it isn't necessary (type is string, not string | null) or has been checked in user code already. It's somewhat a dilemma between efficiency and convenience I guess, and I typically choose the former since the latter can be achieved by wrapping an efficient helper with a convenient function, while a convenient helper cannot be made efficient.

@jtenner
Copy link
Contributor

jtenner commented May 16, 2020

I like this reasoning. Thanks for the explanation.

@dcodeIO dcodeIO deleted the loader-notzero branch June 11, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants