-
Notifications
You must be signed in to change notification settings - Fork 121
Implement binary:match/2 and binary:match/3 #1621
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
Conversation
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.
Let's also rebase on latest main. I added few comments on binary pos len functions introduced with the previous PR. Let's also handle them.
83bfbf1
to
79dfc02
Compare
I'll squash commits after review. |
" true -> io:format(\"Expected ~B, got ~p\n\", [%i, R]) " | ||
"end, " | ||
"erlang:process_flag(trap_exit, false), \n" /* init(3) traps exits */ | ||
"S = try %s:start() of\n" |
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.
What's the purpose of this change?
@pguyot did the last refactoring here, his opinion might be valuable here.
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.
With hard pattern matching in test, we won't catch badmatch
without this try
and while the command will still fail, we won't get nice error message.
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.
understood, sounds like a good idea. Since this change is unrelated to the Implement binary:match/2 and binary:match/3
it would be perfect to have it as a separated commit with this short explanation (we don't need an additional PR, just one more commit).
git log
history should allow us to understand the rationale about why we did a change.
88b8b8d
to
70ee6b9
Compare
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.
Everything good now, just fixup/squash all redundant commits (and let's use clear commit messages).
" true -> io:format(\"Expected ~B, got ~p\n\", [%i, R]) " | ||
"end, " | ||
"erlang:process_flag(trap_exit, false), \n" /* init(3) traps exits */ | ||
"S = try %s:start() of\n" |
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.
understood, sounds like a good idea. Since this change is unrelated to the Implement binary:match/2 and binary:match/3
it would be perfect to have it as a separated commit with this short explanation (we don't need an additional PR, just one more commit).
git log
history should allow us to understand the rationale about why we did a change.
Signed-off-by: Jakub Gonet <[email protected]>
Running tests with beam didn't catch exceptions which resulted in command failing with non-informational error. Now, we catch all exceptions, print them, and exit with error code. Signed-off-by: Jakub Gonet <[email protected]>
5ef2f33
to
846fa18
Compare
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later