Skip to content

Embedded scripts cleanup - #1741

Open
rfbgo wants to merge 5 commits into
Ramble-Project:developfrom
rfbgo:embedded_scripts_dedup
Open

Embedded scripts cleanup#1741
rfbgo wants to merge 5 commits into
Ramble-Project:developfrom
rfbgo:embedded_scripts_dedup

Conversation

@rfbgo

@rfbgo rfbgo commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Previously we had python files (eg _ramble_cleaner.py) hiding as comments which we then echo to file. This PR promotes them to real files and allows them to be copied instead

This means we can do things like lint and reason about them more easily

@rfbgo
rfbgo requested a review from douglasjacobsen August 24, 2026 22:22

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request refactors the workspace utility scripts by moving the inline script strings from cleaner.py and file_editor.py into standalone files under share/ramble/scripts/. The workspace logic now copies these scripts directly using shutil.copyfile, and corresponding tests have been added. Feedback suggests using os.scandir() as a context manager in _ramble_cleaner.py to ensure proper resource cleanup, and defensively checking for None values when loading module specs in _ramble_file_editor.py.

Comment thread share/ramble/scripts/_ramble_cleaner.py
Comment thread share/ramble/scripts/_ramble_file_editor.py
@ramble-pr-bot

ramble-pr-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

Ramble Performance Test Metrics

Results produced with commit: b2e0446

Test Name Outcome Duration (s) Most Recent Run (s) Last 5 Avg (s)
test_analyze_large_file passed 2.1646 2.1057 (279e771) 2.0811
test_large_template_expansion passed 1.1816 1.1903 (279e771) 1.1715
test_many_experiments passed 24.5376 23.4966 (279e771) 23.2766
test_many_objects_defaults passed 13.4906 12.3570 (279e771) 12.2878
test_matrix_filter_perf passed 1.1719 1.0507 (279e771) 1.0456

@rfbgo
rfbgo force-pushed the embedded_scripts_dedup branch from 9dff05d to 776e222 Compare August 24, 2026 22:49
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.05%. Comparing base (96b2062) to head (b2e0446).
⚠️ Report is 7 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1741   +/-   ##
========================================
  Coverage    94.05%   94.05%           
========================================
  Files          369      370    +1     
  Lines        37180    37234   +54     
========================================
+ Hits         34968    35022   +54     
  Misses        2212     2212           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rfbgo
rfbgo requested a review from linsword13 August 25, 2026 14:20
@rfbgo rfbgo changed the title Embedded scripts dedup Embedded scripts cleanup Aug 25, 2026
linsword13
linsword13 previously approved these changes Aug 25, 2026

@linsword13 linsword13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great stuff!

One nit question: are there other checks (like mypy) that should be enabled for these /share/ramble/scripts/** files?

@rfbgo

rfbgo commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Hmm great question. I will push something to enable mypy for those files

@rfbgo
rfbgo requested a review from linsword13 September 1, 2026 12:28

@rfbgo rfbgo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@dcoapp recheck

rfbgo and others added 5 commits September 3, 2026 11:05
Signed-off-by: Bob <robertbird@google.com>
Signed-off-by: Bob <robertbird@google.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Bob <robertbird@google.com>
Signed-off-by: Bob <robertbird@google.com>
Signed-off-by: Bob <robertbird@google.com>
@rfbgo
rfbgo force-pushed the embedded_scripts_dedup branch from b2e0446 to 4632b98 Compare September 3, 2026 17:05
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.

2 participants