-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix parsing of Bindings with Escaped Numbers #533
Conversation
@@ -162,6 +162,16 @@ describe("errors", () => { | |||
); | |||
}); | |||
|
|||
test("works for bindings with escaped numeric bindings", () => { |
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.
Can we add more test cases for this to the list?
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.
}, | ||
}); | ||
|
||
expect(parser.parse("foo['01'].baz").asString()).toBe("foo.01.baz"); |
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.
Do we need to handle cases for parser.parse('foo.01.baz')
? Does that also parse to a string instead of an int?
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.
Good question. Current behavior is that would parsed as an integer and we should probably keep that behavior. that does mean the parsing of that binding needs to probably happen a bit differently than it is now because the context of if a segment was wrapped in the escape syntax doesn't make it down to the binding constructor.
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 make sure we have tests for foo.01.baz
and foo[01].baz
to make sure we maintain existing behavior to be safe
f7b5e84
to
6b2aa5c
Compare
Benchmark Results Main
Branch
Definitely some impact on performance overall but not sure if that can be avoided |
…x/bindings-with-escaped-numbers
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #533 +/- ##
==========================================
- Coverage 91.88% 89.57% -2.31%
==========================================
Files 341 331 -10
Lines 27303 19718 -7585
Branches 1944 1949 +5
==========================================
- Hits 25087 17663 -7424
+ Misses 2202 2041 -161
Partials 14 14 ☔ View full report in Codecov by Sentry. |
Change Type (required)
Indicate the type of change your pull request is:
patch
minor
major
Does your PR have any documentation updates?