Skip to content

Conversation

diemol
Copy link
Member

@diemol diemol commented Oct 16, 2025

User description

There was an issue with the script, and it failed to include the version block at the top of the PDL file. Due to this, the generation of CDP bindings for Python was failing.


PR Type

Bug fix


Description

  • Preserve version block at top of PDL file during concatenation

  • Fix CDP binding generation failure for Python

  • Comment out updates for Java, .NET, Ruby, and JavaScript bindings


Diagram Walkthrough

flowchart LR
  A["Extract version block"] --> B["Concatenate domain PDL files"]
  B --> C["Write version block + domains"]
  C --> D["Generate Python CDP bindings"]
Loading

File Walkthrough

Relevant files
Bug fix
update_cdp.py
Preserve version block in PDL concatenation for Python     

scripts/update_cdp.py

  • Extract and preserve version block from PDL file using regex
  • Prepend version block to concatenated domain content
  • Comment out binding updates for Java, .NET, Ruby, and JavaScript
  • Keep only Python binding update active
+10/-7   

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Oct 16, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 16, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Incomplete parsing

Description: The regex for extracting the version block only matches a single-line pattern and may miss
or partially capture multi-line version blocks, potentially leading to malformed PDL
output; tighten the pattern to capture the full block or validate presence before write.
update_cdp.py [72-73]

Referred Code
version_match = re.search(r"(version\s+major\s+\d+\s+minor\s+\d+)", content)
version_block = version_match.group(1) + "\n\n" if version_match else ""
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
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

Copy link
Contributor

qodo-merge-pro bot commented Oct 16, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Validate required version block

Fail fast with a clear error if the version block is missing or malformed,
instead of silently proceeding with an empty header.

scripts/update_cdp.py [72-73]

 version_match = re.search(r"(version\s+major\s+\d+\s+minor\s+\d+)", content)
-version_block = version_match.group(1) + "\n\n" if version_match else ""
+if not version_match:
+    raise ValueError(f"Missing or invalid version block in {file_path}")
+version_block = version_match.group(1) + "\n\n"
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early to avoid logic errors, especially when relying on regex parsing to extract required blocks.

Low
General
Anchor regex to start of line

To prevent incorrect matches, anchor the version-matching regex to the start of
a line using ^ and the re.MULTILINE flag.

scripts/update_cdp.py [72-73]

-version_match = re.search(r"(version\s+major\s+\d+\s+minor\s+\d+)", content)
+version_match = re.search(r"^(version\s+major\s+\d+\s+minor\s+\d+)", content, re.MULTILINE)
 version_block = version_match.group(1) + "\n\n" if version_match else ""
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly improves the robustness of the regular expression by anchoring it to the start of a line, preventing potential mismatches with similar text in comments or other parts of the file.

Low
  • Update

Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

LGTM

@diemol diemol merged commit 5e82edc into trunk Oct 16, 2025
18 checks passed
@diemol diemol deleted the fixing_cdp_generation_for_python branch October 16, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants