Add a getter and setter for booleans in JSON#6682
Add a getter and setter for booleans in JSON#6682dlang-bot merged 1 commit intodlang:masterfrom Erikvv:feature/json-boolean-getter-setter
Conversation
|
Thanks for your pull request and interest in making D better, @Erikvv! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#6682" |
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
Your pull request looks good to me, modulo some minor comments about the documentation:
std/json.d
Outdated
| } | ||
|
|
||
| /*** | ||
| * Value getter/setter for boolean stored in JSON |
There was a problem hiding this comment.
Missing full stop at the end of the sentence.
std/json.d
Outdated
| /*** | ||
| * Value getter/setter for boolean stored in JSON | ||
| * Throws: `JSONException` for read access if `this.type` is not | ||
| * `JSONType.true_` or `false_`. |
There was a problem hiding this comment.
Prefer spelling out the full name:
`JSONType.true_` or `JSONType.false_`.
There was a problem hiding this comment.
Done. I find the comment has too much implementation details but it's the same in the rest of the module.
|
Since this adds a new public symbol, @andralex will need to sign off on this PR before it can be merged. On the other hand the newly added |
CyberShadow
left a comment
CyberShadow
left a comment
There was a problem hiding this comment.
LGTM too, looks like a no-brainer.
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
Requesting change of signature. Otherwise I'm fine with the addition.
std/json.d
Outdated
| * Throws: `JSONException` for read access if `this.type` is not | ||
| * `JSONType.true_` or `JSONType.false_`. | ||
| */ | ||
| @property inout(bool) boolean() inout pure @safe |
There was a problem hiding this comment.
Why inout? This should suffice: @property bool boolean() const pure @safe. That ensures no surprise with functions that initialize an auto with the result of the call.
There was a problem hiding this comment.
You're right I changed it to const. I'll make another PR to change the other getters as well.
std/json.d
Outdated
| return false; | ||
| default: | ||
| throw new JSONException("JSONValue is not a boolean type"); | ||
| } |
There was a problem hiding this comment.
A three-branch switch seems a bit much to me.
if (type == JSONType.true_) return true;
if (type == JSONType.false_) return false;
throw ...
But... six vs. half a dozen.
…loat Fixes Issue 19899 - std.bitmanip.bitsSet should accept const arguments This also makes `swapEndian` not propagate `const` to its return value which is arguably an improvement (see dlang#6682 (comment)).
5 participants
Works the same as
integer,floatingetc.Seems obvious you need this to use the module in any real-world usage.