Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document null pointer behavior of the loader helpers #1280

Merged
merged 1 commit into from
May 16, 2020
Merged

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.

2 participants