-
Notifications
You must be signed in to change notification settings - Fork 6
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
ref: replace if_parser with esp_bool_parser package #176
Conversation
1a2f8fa
to
a88fa72
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.
LGTM. I think we shall make a few more changes on the esp-bool-parser
package before we merge this PR.
a88fa72
to
8adac3b
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.
LGTM.
dba0a9b
to
d1380bb
Compare
could you also replace those constants in so we could put all the parsing logic in one place. |
f17c37c
to
1485eaf
Compare
@hfudev What about this SHA test? Is it related to this PR? |
Yes the failure is related. I'm afraid we have to update the
The code is We shall somehow rely on the obj.stmt.value, instead of the object. could you help update the sha function and fixing the test cases? |
61bc052
to
67f1ea6
Compare
I used |
67f1ea6
to
92fc2e9
Compare
92fc2e9
to
00f7342
Compare
LGTM! Nice cleanup. Could you help create a follow-up PR to update the documentation to mention that the limitation of "chained operators" has been eliminated? |
Description
We have already moved the
if_parser
logic outside ofidf-build-apps
, so it's time to replace theif_parser
inidf-build-apps
withesp_bool_parser
.Also, I think we can mention
SUPPORTED_TARGET
in theesp_bool_parser
docs, because some libraries will be using it.Related
Testing
Checklist
Before submitting a Pull Request, please ensure the following: