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

Add `in` operator to Bson and Json #1032

Merged
merged 1 commit into from Mar 25, 2015

Conversation

Projects
None yet
4 participants
@schuetzm
Contributor

schuetzm commented Mar 24, 2015

No description provided.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 24, 2015

Member

This unfortunately doesn't work for Bson, because opApply yields a reference to a temporary. This is a good reminder that the signature should be changed to drop the ref, since that requirement should now be gone long enough.

Member

s-ludwig commented Mar 24, 2015

This unfortunately doesn't work for Bson, because opApply yields a reference to a temporary. This is a good reminder that the signature should be changed to drop the ref, since that requirement should now be gone long enough.

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Mar 24, 2015

Contributor

Hmm... okay, is there then another way to check whether a key is in a Bson or Json object, even if it doesn't return a pointer, but just a bool?

Contributor

schuetzm commented Mar 24, 2015

Hmm... okay, is there then another way to check whether a key is in a Bson or Json object, even if it doesn't return a pointer, but just a bool?

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 24, 2015

Contributor

Also, why use the D1 operator overloading style over opBinary ?

Contributor

Geod24 commented Mar 24, 2015

Also, why use the D1 operator overloading style over opBinary ?

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Mar 24, 2015

Contributor

@Geod24 Somehow I didn't think that in would work with opBinaryRight(), but it makes sense.

Contributor

schuetzm commented Mar 24, 2015

@Geod24 Somehow I didn't think that in would work with opBinaryRight(), but it makes sense.

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Mar 24, 2015

Contributor

I changed it to return bool. Is that acceptible?

Contributor

schuetzm commented Mar 24, 2015

I changed it to return bool. Is that acceptible?

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 24, 2015

Contributor

Not to me. Changing an operator meaning / semantic via an overload is not something we should do.

Contributor

Geod24 commented Mar 24, 2015

Not to me. Changing an operator meaning / semantic via an overload is not something we should do.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 24, 2015

Member

Hm, it sounds reasonable to me in general. The only thing it changes about the semantics is that it tightens them. Implicit conversion of T* to bool happens anyway, so there is nothing bad about this as far as I can see.

One thing that concerns me a little bit about in for Bson in general is that it's already O(n) and using in + opIndex is much slower than it should be. A method returning Nullable!Bson could be an alternative here (or letting opIndex return a nullable, but that probably is a breaking change for some obscure reason).

Member

s-ludwig commented Mar 24, 2015

Hm, it sounds reasonable to me in general. The only thing it changes about the semantics is that it tightens them. Implicit conversion of T* to bool happens anyway, so there is nothing bad about this as far as I can see.

One thing that concerns me a little bit about in for Bson in general is that it's already O(n) and using in + opIndex is much slower than it should be. A method returning Nullable!Bson could be an alternative here (or letting opIndex return a nullable, but that probably is a breaking change for some obscure reason).

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Mar 25, 2015

Contributor

Right, in is supposed to be O(1). I added a tryIndex() method that returns a Nullable!Bson. For Json, I left it as is because it can provide a real in operator returning a pointer.

Contributor

schuetzm commented Mar 25, 2015

Right, in is supposed to be O(1). I added a tryIndex() method that returns a Nullable!Bson. For Json, I left it as is because it can provide a real in operator returning a pointer.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 25, 2015

Contributor

Hm, it sounds reasonable to me in general. The only thing it changes about the semantics is that it tightens them. Implicit conversion of T* to bool happens anyway, so there is nothing bad about this as far as I can see.

in is expected to yield a pointer to the underlying value, and if (x in c) is just an use case. Actually, a very common idiom is:

if (auto p = x in b)
    // Use p

It leverage the fact the canFind and find are (often) similar operation, and often performed together. So having an in that returns bool would change in to mean canFind.

That being said, the P.R. now LGTM. Thanks @schuetzm !

Contributor

Geod24 commented Mar 25, 2015

Hm, it sounds reasonable to me in general. The only thing it changes about the semantics is that it tightens them. Implicit conversion of T* to bool happens anyway, so there is nothing bad about this as far as I can see.

in is expected to yield a pointer to the underlying value, and if (x in c) is just an use case. Actually, a very common idiom is:

if (auto p = x in b)
    // Use p

It leverage the fact the canFind and find are (often) similar operation, and often performed together. So having an in that returns bool would change in to mean canFind.

That being said, the P.R. now LGTM. Thanks @schuetzm !

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 25, 2015

Member

Believe me that I'm very well aware of that idiom and the efficiency implications ;) But my point is that the behavior is changed in a completely safe and unsurprising way. You may lose a certain idiom, but that's it. I generally look at such things in a pragmatic and case by case way - different/unexpected results for the same code on the other hand would definitely be a red flag. But the efficiency concern is also valid of course and is the reason why I agree in the end that in is not optimal in this case.

On the other hand tryIndex is also non-standard and doesn't fit with any potential future std.data.bson module, so I'm also not completely happy with that. But I don't see any better solution either, so let's go with it.

Member

s-ludwig commented Mar 25, 2015

Believe me that I'm very well aware of that idiom and the efficiency implications ;) But my point is that the behavior is changed in a completely safe and unsurprising way. You may lose a certain idiom, but that's it. I generally look at such things in a pragmatic and case by case way - different/unexpected results for the same code on the other hand would definitely be a red flag. But the efficiency concern is also valid of course and is the reason why I agree in the end that in is not optimal in this case.

On the other hand tryIndex is also non-standard and doesn't fit with any potential future std.data.bson module, so I'm also not completely happy with that. But I don't see any better solution either, so let's go with it.

s-ludwig added a commit that referenced this pull request Mar 25, 2015

Merge pull request #1032 from schuetzm/opin
Add `in` operator to Bson and Json

@s-ludwig s-ludwig merged commit ad2f4cb into vibe-d:master Mar 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Mar 25, 2015

Contributor

Thanks!

Contributor

schuetzm commented Mar 25, 2015

Thanks!

@schuetzm schuetzm deleted the schuetzm:opin branch Mar 25, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 26, 2015

Member

On issue that I have stumbled over is that the Json version will be successful for if (key in json) when json[key] is of type undefined. Since Json.Type.undefined is used more or less interchangeably with a non-existent field, it may make sense to return null, even if the AA field is set. Otherwise it's easy to introduce bugs with this. Thoughts?

Member

s-ludwig commented Jul 26, 2015

On issue that I have stumbled over is that the Json version will be successful for if (key in json) when json[key] is of type undefined. Since Json.Type.undefined is used more or less interchangeably with a non-existent field, it may make sense to return null, even if the AA field is set. Otherwise it's easy to introduce bugs with this. Thoughts?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 26, 2015

Contributor

Maybe this isn't the best place to put it, but I've been writing too much boilerplate for this. In javascript, you should be able to do json[key][key2][0][key3] and get undefined, whereas in vibe.d you have to write if (json.key.to!bool && json.key.type == Json.Type.object && json.key.key2.to!bool && ....

Contributor

etcimon commented Jul 26, 2015

Maybe this isn't the best place to put it, but I've been writing too much boilerplate for this. In javascript, you should be able to do json[key][key2][0][key3] and get undefined, whereas in vibe.d you have to write if (json.key.to!bool && json.key.type == Json.Type.object && json.key.key2.to!bool && ....

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 26, 2015

Member

For std.data.json I've added an opt method for that purpose: https://github.com/s-ludwig/std_data_json/blob/3efc0600b4f8598dd6ccf897d6140d3351b5ee84/source/stdx/data/json/value.d#L103

For vibe.data.json we have the problem that we basically can't add any methods without silent breakage until opDispatch is removed. Since it has only been marked as scheduled for deprecation in this release, this will take at least 2 more releases :(

Member

s-ludwig commented Jul 26, 2015

For std.data.json I've added an opt method for that purpose: https://github.com/s-ludwig/std_data_json/blob/3efc0600b4f8598dd6ccf897d6140d3351b5ee84/source/stdx/data/json/value.d#L103

For vibe.data.json we have the problem that we basically can't add any methods without silent breakage until opDispatch is removed. Since it has only been marked as scheduled for deprecation in this release, this will take at least 2 more releases :(

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Jul 28, 2015

Contributor

I noticed another unexpected behaviour with vibe.data.json.Json: Undefined is treated like NaN, i.e. it is unequal to anything, including itself. This was surprising to me, especially because it means that two larger Json objects that contain an undefined member, but are otherwise equal, are treated as unequal. If we change the in operator to treat undefined as absent, it should get treated that everywhere for consistency, e.g. {"foo":1,"bar":undefined} should be equal to {"foo":1} (and to itself, of course).

About opDispatch: I think it's still useful. We should just document that we reserve the right to add more methods in the future and will not consider them breaking changes.

Contributor

schuetzm commented Jul 28, 2015

I noticed another unexpected behaviour with vibe.data.json.Json: Undefined is treated like NaN, i.e. it is unequal to anything, including itself. This was surprising to me, especially because it means that two larger Json objects that contain an undefined member, but are otherwise equal, are treated as unequal. If we change the in operator to treat undefined as absent, it should get treated that everywhere for consistency, e.g. {"foo":1,"bar":undefined} should be equal to {"foo":1} (and to itself, of course).

About opDispatch: I think it's still useful. We should just document that we reserve the right to add more methods in the future and will not consider them breaking changes.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 28, 2015

Member

You are right about comparison, it should definitely ignore undefined fields.

opDispatch - I'm convinced at this point that it was a mistake, at least for the DOM type itself, not only because it makes any API change a possibly silent(!) breaking change. What might be interesting would be to have an additional type akin to Adam Ruppe's var that can take the contents of a Json or Bson and provides full simulated dynamic typing. Instead of Json j = parseJson(...); you'd simply write var j = parseJson(...); and be good to go.

Member

s-ludwig commented Jul 28, 2015

You are right about comparison, it should definitely ignore undefined fields.

opDispatch - I'm convinced at this point that it was a mistake, at least for the DOM type itself, not only because it makes any API change a possibly silent(!) breaking change. What might be interesting would be to have an additional type akin to Adam Ruppe's var that can take the contents of a Json or Bson and provides full simulated dynamic typing. Instead of Json j = parseJson(...); you'd simply write var j = parseJson(...); and be good to go.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 28, 2015

Member

BTW, in stdx.data.json, I've completely removed the undefined type. That turned out to be the second mistake to carry over from JavaScript. It just complicates the logic, as well as the observable behavior. Using Nullable (or an Option type) and an explicit opt method is the far better approach. I'd like to transition vibe.data.json to this scheme, too, but I'm not sure about the deprecation path. Probably I'll just deprecate the whole package once std.data.json is in.

Member

s-ludwig commented Jul 28, 2015

BTW, in stdx.data.json, I've completely removed the undefined type. That turned out to be the second mistake to carry over from JavaScript. It just complicates the logic, as well as the observable behavior. Using Nullable (or an Option type) and an explicit opt method is the far better approach. I'd like to transition vibe.data.json to this scheme, too, but I'm not sure about the deprecation path. Probably I'll just deprecate the whole package once std.data.json is in.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 28, 2015

Member

Hmmm, what happened here? I just discovered that Json already had the in operator defined all the time. It also already has the right behavior to return null for undefined fields. I'm simply going to remove the change in this PR and we should be fine.

Member

s-ludwig commented Jul 28, 2015

Hmmm, what happened here? I just discovered that Json already had the in operator defined all the time. It also already has the right behavior to return null for undefined fields. I'm simply going to remove the change in this PR and we should be fine.

s-ludwig added a commit that referenced this pull request Jul 28, 2015

Remove duplicate op "in" from Json. See #1032.
Also adds a documented unit test to nail down the behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment