Skip to content

add IsNull helper method#44

Closed
fuweid wants to merge 1 commit intotidwall:masterfrom
fuweid:add_IsNull_method
Closed

add IsNull helper method#44
fuweid wants to merge 1 commit intotidwall:masterfrom
fuweid:add_IsNull_method

Conversation

@fuweid
Copy link

@fuweid fuweid commented Sep 18, 2017

Hi @tidwall

json allows nullable value in body so that we can't use Exists method for this case.

For now, we can use p.Type == gjson.Null to check the value is NULL. In HTTP PATCH method, it's very common case in implementation. So, could we make it graceful by providing helper IsNull like IsArray? Does it make senses to you?

@tidwall
Copy link
Owner

tidwall commented Oct 14, 2017

Hi @0x04C2

I'm a little worried that IsNull() will create confusion in some cases because it could mean different things to different users.

Takes these two payloads for example:

{"id":100, "first_name":null}
{"id":100}

Your version of gjson.Get(json, "first_name").IsNull() will return true for both cases. But should it?

With the current version of GJSON a user can use:

p := gjson.Get(json,"first_name")
if p.Type == gjson.Null {
        // ... true for both cases
}
if p.Type == gjson.Null && p.Exists() {
        // ... true only for first case
}

I can totally see how one user might desire the first condition as the behavior for IsNull() and another user the second.

I'm inclined to steering clear from an IsNull for now, but I happy to hear further feedback.

@fuweid
Copy link
Author

fuweid commented Oct 17, 2017

Hi @tidwall , sorry for the delay reply.

Basically, I just want to add API to help user to check the null value.

Yes. I agree with you. For the second case you mentioned, the users maybe think that IsNull includes Exists if they don't read the document.

During using gjson, I like that there are very simple, powerful and easy-to-remember APIs.
For this, I'd like to use p.Type == gjson.Null to keep it simple. Thank you for pointing it out. 💯

If there are more and more users which think there is no confusion, we can reconsider it. :)

@bcho
Copy link
Contributor

bcho commented Oct 23, 2017

Maybe naming Exists with IsUndefined is a better solution for distinguishing null and undefined values , but on the other hand, this will break the API (or maybe add IsUndefined as an alias to Exists?)

@tidwall
Copy link
Owner

tidwall commented Nov 13, 2017

@0x04C2 I'm closing this PR for now. I don't think this change is necessary at this time. Thanks for the feedback on this issue.

@tidwall tidwall closed this Nov 13, 2017
@fuweid fuweid deleted the add_IsNull_method branch November 14, 2017 01:53
@fuweid
Copy link
Author

fuweid commented Nov 14, 2017

It makes sense. Thanks

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