-
Notifications
You must be signed in to change notification settings - Fork 79
Bail out of Patterns with too long placeables #395
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
Bail out of Patterns with too long placeables #395
Conversation
I wonder if we should You had concerns around I'm having second thoughts on this, in particular around question like how this would be implemented with Is this specific implementation practically better than what we have? Maybe it's not worth to tweak the status quo right now, and modify the behavior once we have something spec'ed? |
Yes, but I'll need to create a benchmark to verify this. I'll report in #364 or #390, or both.
Yes, my rationale for opening this PR was along the same lines. This isn't perfect, but it is better that what we currently do on master. |
I changed the approach to treating the error as a fatal one, which causes the resolver to instantly bail out of the affected pattern. This provides a much better CPU protection in deeply nested scenarios. I think we could even consider increasing |
Using |
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.
Nice.
Thanks for the review and the suggestion to bail out in this particular case! |
This ports projectfluent/fluent.js#395, which returns with an empty `FluentNone`, and does so early by raising an exception.
* Uplift tests for selector expression validation, fixes #122 * Uplift billion laughs from fluent.js, abort early This ports projectfluent/fluent.js#395, which returns with an empty `FluentNone`, and does so early by raising an exception.
Fix #388.