Skip to content
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

"Strict mode" the same behavior as in ruby's #269

Closed
wants to merge 17 commits into from

Conversation

msangel
Copy link
Collaborator

@msangel msangel commented Mar 31, 2023

As a result of detected problems in #267
todos:

  • error_mode setting
  • remove used alternative in particular cases
  • apply in the parser for the same incorrect expression evaluating in {{ output }} tags as for Comparisons within assign Shopify/liquid#1102 according to existing logic (lax vs strict)
    • apply incorrect expression evaluating
    • lax vs strict
      • parser differentiation in vocabulary
      • parser implementation detect garbage and record error (lax vs warn)
      • Where filter still rely on exact flavour
      • more test coverage!
  • Flavor.filters vs ParserSettings.filters differentiation, make more use of new Filters class(immutable)
  • get rid of static context in Filter
  • get rid of static context in Insertion
  • ProtectionSettings is not immutable right now as the counter live in it, counter should be moved in context (probably as registry)
  • in LIPQ flavor enable expected behavior (expressions are evaluated)
  • undo string-to-digit conversion as for Gt, GtEq, Lt and LtEq and allow it only for the same family type(numbers, dates, strings)
  • more unit tests

* </code>
*/
public enum ErrorMode {
strict,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they're enum constants, they should be all-uppercase (STRICT, WARN, LAX)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I simply wanted to follow ruby, so I treat ruby literal as a lower-case enum. Still, changing this probably makes more sense.

Copy link
Contributor

@kohlschuetter kohlschuetter Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'm hoping we can add some automated checks with PMD, Checkstyle, Spotbugs, etc. They're really helpful. That would come with either adhering more to the standard Java conventions or adding a bunch of exceptions. I'd lean on the former, with very few exceptions only.

@Deprecated
public TemplateContext(TemplateContext parent, Map<String, Object> variables) {
this(variables, parent);
public LiquidSupport evaluate(final Object variable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so much cleaner 👍

@kohlschuetter
Copy link
Contributor

kohlschuetter commented Jun 27, 2023

This looks good overall. Even if not all to-dos are met, tests pass. The earlier we get this in the better :) 👍

Please see my additional commit below for a regression I just found after clearing up all the other warnings.

kohlschuetter added a commit to kohlschuetter/Liqp that referenced this pull request Jun 27, 2023
antlr rightfully complains: "rule output contains an optional block with
at least one alternative that can match an empty string" (which hints at
a potential performance problem).

This is due to "not_out_end" matching with "*" (which can yield an empty
string), even though we already specify "?" at "unparsed=not_out_end?".

Fix the parser grammar, and adjust two test cases where we actually
tested for that undesired behavior.

Also see
https://stackoverflow.com/questions/26041293/antlr-4-warning-rule-contains-an-optional-block-with-at-least-one-alternative
for an explanation.

Regression introduced in bkiers#269
@msangel msangel closed this Jul 29, 2023
@msangel msangel deleted the feature/strict_mode-setting branch November 6, 2023 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants