-
Notifications
You must be signed in to change notification settings - Fork 503
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 support for bitwise operators. #20
Conversation
Input: "true | false", | ||
Expected: INVALID_TOKEN_KIND, | ||
}, | ||
// ParsingFailureTest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These test don't hold true anymore as &
and |
are valid operators now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. You can straight-up delete these lines, the main case they covered was making sure the parser wasn't short-circuiting things. There's no future need for them with this change.
Readme needs to be updated |
The only reason I'm hesitant to merge the XOR/exponent change (even though it's the semantics that seem the most reasonable) is because I suspect people clone from master branch in their builds. There's ~25 clones per weekday, and I'd hate to cause someone an outage because the meaning of It might be that they're cloning a specific tag or commit (i'd hope they were), but I can't tell from github's metrics. I was going to look into that more later this week. |
As a total sidenote, the commit is listed as authored by braintreeps. Doesn't matter either way to me, but i suspect you might want the "wmiller" account to be listed as a contributor :P |
Totally correct on the user, work bleeding over 🎱 Yeah I totally get the hesitation on XOR, its a tough call. Is they're any reasonable way we could provide backwards compatibility... 💭 |
My only thought is tag this a 2.0 and make it clear in the README that breaking changes have been made to exponents. OR We provide some sort of feature switching mechanism (but that sounds terrible), defaults to pre-change behavior? |
3588ac7
to
28952ce
Compare
…d `~` (not) are now supported. Float64 values are truncated to int64 then converted back. The `exponent` operator is now `**`.
28952ce
to
c6a6abe
Compare
I didn't end up finding a way to communicate this change very well, so cutting a 2.0, making notes, and letting downstreams take the hit if they don't vendor seems to be the right way to go about it. Planning on merging this change and the work done in #21 to make a 2.0 release sometime tomorrow night. |
&
(and),|
(or),^
(xor), and~
(not) are now supported.Float64 values are truncated to int64 then converted back.
The
exponent
operator is now**
.