Refactored .sh script to Python#142
Conversation
|
Approved. This PR cleanly replaces the Unix-only shell script with a cross-platform Python script, improving Windows compatibility without changing core behaviour. The implementation is clear and well-structured. |
cameronhum
left a comment
cameronhum
left a comment
There was a problem hiding this comment.
ran the python code with no errors and everything seems to build/copy as intended. made a few suggestions to change. I am happy to approve after they are resolved or your current solution is justified as the functionality within this file and the original .sh is functionally the same
| import shutil | ||
|
|
||
| # Paths are defined relative to this script's location | ||
| HERE = Path(__file__).resolve().parent |
There was a problem hiding this comment.
This is a bit confusing. maybe we can name it something more clear like SCRIPT_DIR etc. that way it follows the standards in the below constants and is more explanatory as to what "here" means
| TARGET_DIR = HERE.parent / "CSharpWasmExpo" / "bin" | ||
| FRAMEWORK_SRC = BIN_DIR / "wwwroot" / "_framework" | ||
| FRAMEWORK_DEST = HERE.parent / "CSharpWasmExpo" / "wwwroot" / "_framework" | ||
|
|
There was a problem hiding this comment.
Here and a few other places some comments are included in the original which are not explained here. I would recommend copying them for consistency. however some you replaced with function names which I believe make it much more readable and understandable
| FRAMEWORK_DEST.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Copy contents of _framework (not the directory itself) | ||
| for item in FRAMEWORK_SRC.iterdir(): |
There was a problem hiding this comment.
This loop is a bit complex for the task. also the rmtree is there but this isn't present in the original. If this is for stale files from an earlier build then we should delete everything in the folder to start with since files that aren't in the new build but were in old wont be deleted and will remain there forever.
however if not then rmtree should be removed.
also to address the complexity of the loop and better follow the cp -r from the original you can add this flag to the end of copytree dirs_exist_ok=True and copy the full directory with no loop. otherwise if you are going ahead with removing stale files/folder structures then you could remove the full loop, remove the FRAMEWORK_DEST.mkdir(parents=True, exist_ok=True) and just do a copytree allowing it to make the directory and copy the contents with no previous build beforehand
3 participants
Description
Refactored buildAndCopy.sh script to Python, have not deleted the original script, have preserved identical build and copy behavior.
Fixes # (issue)
Replaces Unix-only C# install shell script with cross-platform Python implementation to fix Windows compatibility.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I tested by running the script locally to see it completes as expected without any errors and behaves the same as the original script. Can test by cd to CSharpWasm and running the script via python buildAndCopy.py
Testing Checklist
Not needed.
Checklist