Skip to content

Fix #151: Allow mixed comma and whitespace separation in transform pa…#152

Open
shiinapple wants to merge 2 commits intoweisJ:masterfrom
shiinapple:fix-transform-comma
Open

Fix #151: Allow mixed comma and whitespace separation in transform pa…#152
shiinapple wants to merge 2 commits intoweisJ:masterfrom
shiinapple:fix-transform-comma

Conversation

@shiinapple
Copy link

Modified ParserUtil.java to add a guard if (i - start > 0) before adding tokens to the list. This ensures that consecutive separators (e.g., "-1 , 1") do not produce empty strings, which previously caused parsing failures.

Added two tests in AttributeParserTest.java:

  • scale(-1, 1) : Comma separator.
  • scale(-1 , 1) : Mixed space and comma separator.
    Both tests now pass correctly.

This is my first pr. Please let me know if anything needs to be adjusted!

ListSplitter.SplitResult result = splitter.testChar(c, i - start);
if (result.shouldSplit()) {
list.add(value.substring(start, i));
if (i - start > 0) list.add(value.substring(start, i));
Copy link
Owner

Choose a reason for hiding this comment

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

I think this might allow 1,,1 to be parsed as a list of only two strings, while it should really contain the empty string. Can we instead make sure to only avoid splitting while we are seeing whitespace directly after encountering a non whitespace delimiter?

@shiinapple
Copy link
Author

Thanks for the review! I’ve reconsidered the splitting rules for transforms and reworked the implementation accordingly.

Changes

  1. Cached splitOnWhitespace() in a local variable to avoid repeated calls in the loop.
  2. Introduced a lastSplitWasWhiteSpace flag and adjusted the splitting logic so that:
    • Empty entries created by consecutive non‑whitespace separators are preserved (e.g. "1,,1" with SeparatorMode.COMMA_ONLY now produces ["1", "", "1"] ).
    • Only the “whitespace immediately followed by a separator” case avoids creating an extra empty token (e.g. "(-1 , 1)" no longer produces a spurious empty element).
      Verification
  • Added two focused test cases in AttributeParserTest and confirmed they pass.

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

Comments