Skip to content

Conversation

@krystophny
Copy link
Member

@krystophny krystophny commented Oct 12, 2025

No description provided.

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Oct 12, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Oct 12, 2025

PR Code Suggestions ✨

Latest suggestions up to ac77f26

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid formatting inside strings/comments
Suggestion Impact:The condition for formatting '//' was extended to include context checks: level == 0 and pos > end_of_delim, preventing changes inside strings/comments.

code diff:

         # format string concatenation operator '//'
-        if char == '/' and line[pos:pos + 2] == "//" and (pos == 0 or line[pos - 1] != '/'):
+        if (char == '/' and line[pos:pos + 2] == "//" and (pos == 0 or line[pos - 1] != '/')
+                and level == 0 and pos > end_of_delim):
             lhs = line_ftd[:pos + offset]
             rhs = line_ftd[pos + 2 + offset:]
             line_ftd = lhs.rstrip(' ') \
@@ -1219,7 +1220,6 @@
                     + "//" \
                     + ' ' * spacey[10] \
                     + rhs.lstrip(' ')
-            line_ftd = line_ftd.rstrip(' ')
 

Prevent incorrect formatting of the // operator when it appears inside string
literals or comments by adding checks for the current parsing context (level and
end_of_delim).

fprettify/init.py [1213-1222]

 # format string concatenation operator '//'
-if char == '/' and line[pos:pos + 2] == "//" and (pos == 0 or line[pos - 1] != '/'):
+if (char == '/'
+    and line[pos:pos + 2] == "//"
+    and (pos == 0 or line[pos - 1] != '/')
+    and level == 0
+    and (end_of_delim < pos or end_of_delim is None)):
     lhs = line_ftd[:pos + offset]
     rhs = line_ftd[pos + 2 + offset:]
     line_ftd = lhs.rstrip(' ') \
             + ' ' * spacey[10] \
             + "//" \
             + ' ' * spacey[10] \
             + rhs.lstrip(' ')
     line_ftd = line_ftd.rstrip(' ')

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical bug where the new formatting logic for // would incorrectly modify string literals or comments containing //, potentially corrupting user code.

High
General
Enable concat spacing in presets
Suggestion Impact:The commit updated the spacey preset for whitespace level 4 to enable spacing around the concat operator (changing the last 0 to 1). It did not modify levels 2 and 3 as suggested, but partially implemented the intention.

code diff:

     elif whitespace == 4:
-        spacey = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0]
+        spacey = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]

Update the spacey presets to enable whitespace for the concatenation operator by
default for whitespace levels 2, 3, and 4 to ensure consistent behavior with
other operators.

fprettify/init.py [1050-1061]

 if whitespace == 0:
     spacey = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
 elif whitespace == 1:
     spacey = [1, 1, 1, 1, 0, 0, 1, 0, 1, 1, 0]
 elif whitespace == 2:
-    spacey = [1, 1, 1, 1, 1, 0, 1, 0, 1, 1, 0]
+    spacey = [1, 1, 1, 1, 1, 0, 1, 0, 1, 1, 1]
 elif whitespace == 3:
-    spacey = [1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 0]
+    spacey = [1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1]
 elif whitespace == 4:
-    spacey = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0]
+    spacey = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
 else:
     raise NotImplementedError("unknown value for whitespace")

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the default behavior for the new concat option is inconsistent with other operators in higher whitespace presets, and the proposed change improves usability and consistency.

Low
Incremental [*]
Document the default behavior

Update the README.md to clarify the default behavior for the --whitespace-concat
flag. Specify that it is disabled by default unless a preset enables it or the
flag is used explicitly.

README.md [112-113]

 Notable toggles include `--whitespace-concat`, which controls whether the
-string concatenation operator `//` is surrounded by spaces when formatting.
+string concatenation operator `//` is surrounded by spaces when formatting
+(default: disabled unless enabled by preset or explicitly with this flag).
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: This is a good suggestion to improve the documentation by clarifying the default behavior of the new --whitespace-concat flag, enhancing user experience.

Low
  • Update

Previous suggestions

Suggestions up to commit e07a55f
CategorySuggestion                                                                                                                                    Impact
High-level
Relocate misplaced operator formatting logic

The formatting logic for the // operator is misplaced in the global scope of
fprettify/init.py. It should be moved into a line processing function like
add_whitespace_charwise to function correctly and avoid runtime errors.

Examples:

fprettify/__init__.py [2160-2169]
        # format string concatenation operator '//'
        if char == '/' and line[pos:pos+2] == "//" and (pos == 0 or line[pos-1] != '/'):
            lhs = line_ftd[:pos + offset]
            rhs = line_ftd[pos + 2 + offset:]
            line_ftd = lhs.rstrip(' ') \
                    + ' ' * spacey[10] \
                    + "//" \
                    + ' ' * spacey[10] \
                    + rhs.lstrip(' ')
            line_ftd = line_ftd.rstrip(' ')

Solution Walkthrough:

Before:

def run(argv=sys.argv):
    # ... function setup ...
    for path in args.path:
        try:
            # ... reformat file ...
            reformat_ffile_combined(...)
        except FprettifyException as e:
            # ... error handling ...

    # Misplaced formatting logic outside any relevant loop
    if char == '/' and line[pos:pos+2] == "//" and ...:
        # ... formatting logic using undefined variables 'char', 'line', 'pos', etc.
        ...

After:

def add_whitespace_charwise(line, spacey, ...):
    # ... function setup ...
    for pos, char in enumerate(line):
        # ... existing char-by-char formatting logic ...

        # Correctly placed formatting logic
        if char == '/' and line[pos:pos+2] == "//" and ...:
            # ... formatting logic for '//' operator ...
            # This logic should be integrated here.
            ...
            continue # or adjust loop variables

    return line_ftd
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical flaw where the new formatting logic is placed in the wrong scope within the run function, making the entire feature non-functional and guaranteed to cause a runtime error due to undefined variables.

High
Possible issue
Move misplaced code into a function

The new code for formatting the string concatenation operator // is in the wrong
scope and will cause a NameError. Move this logic into the
add_whitespace_charwise function where the required variables are defined.

fprettify/init.py [2156-2169]

                               indent_mod=not file_args.disable_indent_mod)
          except FprettifyException as e:
              log_exception(e, "Fatal error occured")
              sys.exit(1)
-    # format string concatenation operator '//'
-    if char == '/' and line[pos:pos+2] == "//" and (pos == 0 or line[pos-1] != '/'):
-        lhs = line_ftd[:pos + offset]
-        rhs = line_ftd[pos + 2 + offset:]
-        line_ftd = lhs.rstrip(' ') \
-                + ' ' * spacey[10] \
-                + "//" \
-                + ' ' * spacey[10] \
-                + rhs.lstrip(' ')
-        line_ftd = line_ftd.rstrip(' ')
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies that the new code block is misplaced, which will cause a NameError at runtime, making the new feature non-functional.

High
Remove unsafe trailing whitespace stripping

Remove the line_ftd.rstrip(' ') call from the string concatenation formatting
logic. This call is unsafe as it can strip significant whitespace from string
literals and is redundant.

fprettify/init.py [2160-2169]

 # format string concatenation operator '//'
 if char == '/' and line[pos:pos+2] == "//" and (pos == 0 or line[pos-1] != '/'):
     lhs = line_ftd[:pos + offset]
     rhs = line_ftd[pos + 2 + offset:]
     line_ftd = lhs.rstrip(' ') \
             + ' ' * spacey[10] \
             + "//" \
             + ' ' * spacey[10] \
             + rhs.lstrip(' ')
-    line_ftd = line_ftd.rstrip(' ')
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that calling rstrip() is risky as it could incorrectly strip whitespace from a string literal, and it is also redundant.

Medium

@krystophny krystophny force-pushed the codex/whitespace-concat branch from 84f4908 to ac77f26 Compare October 12, 2025 13:09
Address Qodo review comments:
- Add level and end_of_delim checks to avoid formatting // inside strings/comments
- Remove unsafe trailing rstrip that could strip string literal whitespace
- Enable concat spacing at whitespace level 4
- Add comprehensive test coverage for concat operator formatting
@krystophny krystophny requested a review from zandivx October 15, 2025 08:27
@zandivx zandivx merged commit 49dbd73 into master Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants