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

Fix isNullish to not consider an empty string as Null. #108

Closed
wants to merge 1 commit into from

Conversation

augustoroman
Copy link
Contributor

An empty string is still a perfectly valid string, in both situations:
(1) An empty string is a valid string argument to a handler.
(2) An empty string is a valid non-null result from a handler.

I also removed the unused and unnecessary PrivateName value from the
NonNull struct -- it served no purpose and screwed up tests when isNullish
was fixed.

See also #107

An empty string is still a perfectly valid string, in both situations:
(1) An empty string is a valid string argument to a handler.
(2) An empty string is a valid non-null result from a handler.

I also removed the unused and unnecessary PrivateName value from the
NonNull struct -- it served no purpose and screwed up tests when isNullish
was fixed.
@franklinkim
Copy link

+1

@Matthias247
Copy link

@sogko Is there any reason not to merge this apart from the need to rebase? It's currently a little bit annoying that no empty strings are supported.

@kyteague
Copy link

Like @Matthias247 said, any reason this major bug fix has not been merged?

@rogchap
Copy link

rogchap commented Nov 2, 2016

Would love to see this fix land.
Maybe we need more people to be core contributors @sogko @chris-ramon ??
Not much activity for a while on this repo.

@solher
Copy link

solher commented Dec 24, 2016

Agreed. Any news on this one? @sogko

@edast
Copy link

edast commented Dec 27, 2016

+1
can this be merged soon?

@martynasb
Copy link

+1 Please land this

@bbuck
Copy link
Collaborator

bbuck commented Mar 23, 2017

@sogko @chris-ramon I think we should cleanup/update this PR (or create a fresh one) and get this change rolling along. The behavior can definitely be confusing, but it's not a small change (as far as who it reaches) as it's a breaking API change if people are depending on the current behavior. But after discussion in #183 I'm convinced that "" should not be considered as null unless we should also consider other zero values as null as well.

@chris-ramon
Copy link
Member

closing this one in favor of #228 — thanks a lot @augustoroman 👍

@chris-ramon chris-ramon closed this Aug 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants