Skip to content

Use argparse in plain_bpmf_compiler - #762

Merged
zonble merged 2 commits into
openvanilla:masterfrom
lukhnos:plain-bpmf-cooker
Dec 14, 2025
Merged

Use argparse in plain_bpmf_compiler#762
zonble merged 2 commits into
openvanilla:masterfrom
lukhnos:plain-bpmf-cooker

Conversation

@lukhnos

@lukhnos lukhnos commented Dec 12, 2025

Copy link
Copy Markdown
Contributor

A small PR to continue the work of refactoring the curation (phrase database compiler) code. Also using this to test how much my previous PRs have tuned the automated code review comments.

Copilot AI review requested due to automatic review settings December 12, 2025 05:10

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors plain_bpmf_compiler.py to use argparse for command-line argument parsing, which improves the script's usability and clarity. The core logic has been moved into a cook function, separating it from the argument parsing. My main feedback is regarding resource management: the script opens files without ensuring they are closed, which can lead to resource leaks. Using with statements for file handling is the recommended practice to avoid this.

Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py Outdated

Copilot AI left a comment

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.

Pull request overview

This PR refactors plain_bpmf_compiler.py to use argparse for command-line argument handling, aligning it with the pattern already established in main_compiler.py. The refactoring improves maintainability by separating the business logic into a cook() function and using modern argument parsing instead of manual sys.argv handling.

  • Refactored command-line parsing from manual sys.argv access to argparse with named arguments
  • Extracted compilation logic into a cook() function that accepts file paths as parameters
  • Updated the Makefile invocation to use the new named argument format

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
Source/Data/curation/compilers/plain_bpmf_compiler.py Refactored to use argparse with named arguments and extracted a cook() function, following the pattern from main_compiler.py
Source/Data/Makefile Updated the plain_bpmf_compiler invocation to use named arguments matching the new argparse interface

Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py Outdated
Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py Outdated
Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py Outdated
@lukhnos

lukhnos commented Dec 12, 2025

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors plain_bpmf_compiler.py to use argparse and with statements, which are great improvements for robustness and readability. The logic is also nicely split into cook and main functions. My review includes one high-severity suggestion to address a memory-efficiency regression introduced when reading one of the input files, ensuring the script remains performant even with large datasets.

Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@lukhnos

lukhnos commented Dec 12, 2025

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request is a nice refactoring of the plain_bpmf_compiler.py script. Using argparse for command-line arguments is a significant improvement in usability and clarity over direct sys.argv access. The separation of the main logic into a cook function and the use of with statements for file handling make the code more robust and Pythonic. The corresponding changes in the Makefile are also correct. I've identified a significant portability issue where file encodings are not specified, which could cause the script to fail on different operating systems. I've added comments with suggestions to address this. Overall, this is a valuable improvement to the codebase.

Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py
Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py
Comment thread Source/Data/curation/compilers/plain_bpmf_compiler.py

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@zonble

zonble commented Dec 14, 2025

Copy link
Copy Markdown
Contributor

Look good. Thanks!

@zonble
zonble merged commit f6d2599 into openvanilla:master Dec 14, 2025
13 checks passed
@lukhnos
lukhnos deleted the plain-bpmf-cooker branch January 11, 2026 00:52
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.

3 participants