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

Important parentheses are not being removed #29

Merged
merged 2 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions src/tctools/format/format_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,12 @@ def _pad_to_indent_level(self, line: str, tab_index: int):


class FormatConditionalParentheses(FormattingRule):
"""Formatter to make uses of parentheses inside IF, CASE and WHILE consistent."""
"""Formatter to make uses of parentheses inside IF, CASE and WHILE consistent.

First regex is used to find potential corrections, which are then investigated
by a loop to make sure parentheses remain matching and no essential parentheses
are removed.
"""

def __init__(self, *args):
super().__init__(*args)
Expand All @@ -377,8 +382,7 @@ def __init__(self, *args):
"twincat_parentheses_conditionals", value_type=bool
)

# Regex to find conditional inside single lines:

# Regex to find likely missing parentheses:
self._re_needs_parentheses = re.compile(
r"""
# Look for start of string or new line:
Expand All @@ -395,11 +399,12 @@ def __init__(self, *args):
re.VERBOSE | re.MULTILINE,
)

# Regex to find likely redundant parentheses:
self._re_removes_parentheses = re.compile(
r"""
# Look for start of string or new line:
^
# Match IF with surrounding ws:
# Match keyword with surrounding ws:
\s*(?:IF|WHILE|CASE)\s*
# Match any characters within ():
\((.+)\)
Expand All @@ -413,10 +418,11 @@ def format(self, content: List[str], kind: Optional[Kind] = None):
if self._parentheses is None:
return # Nothing to do

if self._parentheses:
pattern = self._re_needs_parentheses
else:
pattern = self._re_removes_parentheses
pattern = (
self._re_needs_parentheses
if self._parentheses
else self._re_removes_parentheses
)

for i, line in enumerate(content):
# Do a manual match + replace, instead of e.g. subn(), because we might
Expand All @@ -429,6 +435,13 @@ def format(self, content: List[str], kind: Optional[Kind] = None):
if self._parentheses:
condition = "(" + condition + ")"
else:
# These parentheses could be of importance, check the leading
# "(" is not part of a smaller sub-statement:
if any(
level < 0 for _, level in self.find_and_match_braces(condition)
):
continue

prefix = prefix[:-1] # Remove parentheses
suffix = suffix[1:]

Expand All @@ -448,3 +461,23 @@ def format(self, content: List[str], kind: Optional[Kind] = None):
)

content[i] = prefix + condition + suffix

@staticmethod
def find_and_match_braces(
text: str, brace_left: str = "(", brace_right: str = ")"
) -> Tuple[int, int]:
"""Step through braces in a string.

Note that levels can step into negative.

:return: Tuple of (strpos, level), where strpos is the zero-index position of
the brace itself and level is the nested level it indicates
"""
level = 0
re_any_brace = re.compile(r"[" + brace_left + brace_right + "]")
for match in re_any_brace.finditer(text):
if match.group() == brace_left:
level += 1 # Nest deeper
else:
level -= 1 # Nest back out
yield match.start(), level
50 changes: 29 additions & 21 deletions tests/test_formatting_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def test_variable_align(content, expected, settings):


content_parentheses = [
(
( # Regular IF:
[
"IF inputs.button = 1 THEN\n",
" output.led := 1;\n",
Expand All @@ -286,67 +286,75 @@ def test_variable_align(content, expected, settings):
"END_IF\n",
],
),
(
( # Regular IF with comment (comment should be unchanged):
[
"IF inputs.button = 1 THEN // comment!\n",
],
[
"IF (inputs.button = 1) THEN // comment!\n",
],
),
(
( # IF involving a function call (function parentheses should be unchanged):
[
"IF func(arg1 := 1, args2 := func2()) THEN\n",
],
[
"IF (func(arg1 := 1, args2 := func2())) THEN\n",
],
),
(
( # WHILE involving function call (function parentheses should be unchanged):
[
"WHILE func() DO // comment!\n",
],
[
"WHILE (func()) DO // comment!\n",
],
),
(
( # Regular CASE (part of SWITCH):
[
"CASE idx OF\n",
],
[
"CASE (idx) OF\n",
],
),
# ( # This case fails, because we cannot identify matching parentheses:
# [
# "IF (1+1)*2 = 3*(x-1) THEN\n",
# ],
# [
# "IF ((1+1)*2 = 3*(x-1)) THEN\n",
# ],
# ),
( # IF involving compound statement, parentheses should not be removed at all:
[
"IF (var1 OR var2) AND (var3 OR var4) THEN\n",
],
[
"IF (var1 OR var2) AND (var3 OR var4) THEN\n",
],
),
( # IF involving math statements, parentheses should not be removed at all:
[
"IF (1+1)*2 = 3*(x-1) THEN\n",
],
[
"IF (1+1)*2 = 3*(x-1) THEN\n",
],
),
]


@pytest.mark.parametrize("content,expected", content_parentheses)
def test_parentheses_add(content, expected):
@pytest.mark.parametrize("no_parentheses,parentheses", content_parentheses)
def test_parentheses_add(no_parentheses, parentheses):
rule = format_rules.FormatConditionalParentheses(
{"twincat_parentheses_conditionals": True}
)
content_new = content.copy()
content_new = no_parentheses.copy()
rule.format(content_new)
assert content_new == expected
assert content_new == parentheses


@pytest.mark.parametrize("expected,content", content_parentheses)
def test_parentheses_remove(expected, content):
@pytest.mark.parametrize("no_parentheses,parentheses", content_parentheses)
def test_parentheses_remove(no_parentheses, parentheses):
rule = format_rules.FormatConditionalParentheses(
{"twincat_parentheses_conditionals": False}
)
content_new = content.copy()
content_new = parentheses.copy()
rule.format(content_new)
assert content_new == expected
assert content_new == no_parentheses


def test_parentheses_remove_no_ws():
Expand Down
Loading