Skip to content

Update StringVar to match Python str behavior (#5417)#6596

Open
ubaidrmn wants to merge 1 commit into
reflex-dev:mainfrom
ubaidrmn:fix-5417
Open

Update StringVar to match Python str behavior (#5417)#6596
ubaidrmn wants to merge 1 commit into
reflex-dev:mainfrom
ubaidrmn:fix-5417

Conversation

@ubaidrmn
Copy link
Copy Markdown

@ubaidrmn ubaidrmn commented Jun 2, 2026

Add missing lstrip and rstrip methods to align with Python's str interface. Update strip method to accept a chars argument, consistent with str.strip. Closes #5417.

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Add missing `lstrip` and `rstrip` methods to align with Python's `str` interface. Update `strip` method to accept a `chars` argument, consistent with `str.strip`.
@ubaidrmn ubaidrmn requested a review from a team as a code owner June 2, 2026 13:01
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

This PR adds lstrip and rstrip methods to StringVar and extends strip to accept an optional chars argument, aligning StringVar with Python's str interface. The zero-argument (whitespace) path is correct, but the chars path has two functional defects in the generated JavaScript.

  • When chars is a Python str, the @var_operation wrapper serialises it as a JS string literal (e.g., \"abc\"). That literal β€” quotes included β€” is then placed inside the regex character class, producing /^[\"abc\"]+.../ instead of /^[abc]+.../, so the quotes become characters that are also stripped.
  • When chars is a StringVar (a runtime state variable), the JS variable expression is embedded verbatim in the static regex literal, so the characters of the expression's name are used as the strip set rather than the variable's value at runtime.
  • Special regex characters (], \\, ^, -) inside chars are never escaped, so e.g. strip(\"a-z\") silently strips a range instead of those literal characters.

Confidence Score: 3/5

Safe to merge only for the whitespace-strip path; calling strip/lstrip/rstrip with a chars argument produces incorrect JavaScript and would silently give wrong results for users.

The zero-argument whitespace-trimming path works correctly and the new method signatures are sound. However, every code path that goes through the regex branch (i.e., any call that passes a chars value) will generate broken JavaScript: string literals carry their surrounding quotes into the character class, and state-variable references are embedded statically into the regex literal rather than used as a dynamic value. These are silent runtime-logic failures that would be hard for app authors to debug.

packages/reflex-base/src/reflex_base/vars/sequence.py β€” specifically the regex-construction branches in string_strip_operation, string_lstrip_operation, and string_rstrip_operation.

Important Files Changed

Filename Overview
packages/reflex-base/src/reflex_base/vars/sequence.py Adds lstrip/rstrip methods and chars argument to strip; the regex construction for the chars path is broken: literal str args gain surrounding quotes when serialised as a LiteralVar, and StringVar args are embedded verbatim in the static regex literal instead of being used dynamically.
tests/units/test_var.py Adds whitespace-only assertions for new lstrip/rstrip methods; no test coverage for the chars argument path.
packages/reflex-base/news/5417.feature.md Changelog entry accurately describes the new lstrip/rstrip methods and the updated strip signature.

Comments Outside Diff (1)

  1. tests/units/test_var.py, line 1001-1012 (link)

    P2 No tests for the chars argument path

    The new tests only cover the zero-argument (whitespace) case for lstrip, strip, and rstrip. There are no assertions for passing a chars string, so the incorrectly-quoted regex discussed in the companion comment would not be caught by the test suite.

Reviews (1): Last reviewed commit: "Update `StringVar` to match Python `str`..." | Re-trigger Greptile

Comment on lines +1017 to +1020
return var_operation_return(
js_expression=f"{string}.replace(/^[{chars}]+|[{chars}]+$/g, '')",
var_type=str,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Quoted string literal injected directly into regex character class

When chars is a plain Python str (e.g., "abc"), the @var_operation wrapper converts it to LiteralVar.create("abc"), whose JavaScript serialization is "abc" (with quotes). Interpolating that into the regex literal produces /^["abc"]+.../ β€” the double-quote characters become literal members of the character class, so .strip("abc") would also strip " from the string.

For a StringVar state-variable, the JS variable expression (e.g., state.chars) is embedded verbatim inside the literal regex /.../, which is static syntax β€” it does not reference the variable at runtime and instead treats the characters of the expression name as the set to strip. The same problem exists identically in string_lstrip_operation and string_rstrip_operation.

Comment on lines +1017 to +1020
return var_operation_return(
js_expression=f"{string}.replace(/^[{chars}]+|[{chars}]+$/g, '')",
var_type=str,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Regex special characters in chars are not escaped

Characters such as ], \, ^, and - carry special meaning inside a regex character class. If chars contains any of them (e.g., strip("a-z") would make - a range operator), the generated JavaScript regex will silently behave differently from Python's character-set strip semantics. There is no escaping of these characters before they are placed between [ and ]. The same applies to string_lstrip_operation and string_rstrip_operation.

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.

StringVar.strip() takes 1 positional argument but 2 were given

1 participant