[Review Only - Do Not Merge] Adham/scripts#11
Conversation
|
Hi @AdhamEA - please see the review comments I have made in this pull request. Best way to look at it is with the "Files" tab. You can open a pull request with your branch next time, and I will see it and be able to give you a code review. You can do this even if you are not ready to merge. Just add something like [Review Only] to the title. |
tools python/deploy_skeleton.py
Outdated
| from git import Repo | ||
|
|
||
|
|
||
| USE_ADR=0 |
There was a problem hiding this comment.
Hey Adham,
For variable names, you can use lowercase (use_adr). The upper case is a convention that came from Bash files, but isn't necessary here.
This seems like a reasonable style guide to follow: https://peps.python.org/pep-0008/#global-variable-names
Function names should be lowercase, with words separated by underscores as necessary to improve readability.
Variable names follow the same convention as function names.
tools python/deploy_skeleton.py
Outdated
|
|
||
| #variable=len(opts) | ||
| #for i in range(1,variable) : | ||
| # del opts[i] |
There was a problem hiding this comment.
This can be deleted before merging.
tools python/deploy_skeleton.py
Outdated
| #for i in range(1,variable) : | ||
| # del opts[i] | ||
|
|
||
| CHECK_DIR="c:/subprojects" |
There was a problem hiding this comment.
For readability purposes, I prefer having a space around the "="
CHECK_DIR = "c:/subprojects"
But also, this variable should not be hardcoded to that particular folder, right?
tools python/deploy_skeleton.py
Outdated
|
|
||
| DEST_DIR=args[0] | ||
| DEST_PATH_2= os.path.exists(f"{args[0]}") | ||
| if(DEST_PATH_2==False): |
There was a problem hiding this comment.
Since you have a DEST_DIR variable, you should reference that instead of args[0] int his section of code.
There was a problem hiding this comment.
you mean instead of f"args[0]" i need to put f"{DEST_DIR}" ? right ?
tools python/deploy_skeleton.py
Outdated
| CHECK_PATH_1= os.path.exists(f"{CHECK_DIR}/Tools") | ||
| if(CHECK_PATH_1==False): | ||
| CHECK_DIR=os.chdir(f"{CHECK_DIR}") | ||
| CHECK_PATH_1= os.path.exists(f"{CHECK_DIR}") |
There was a problem hiding this comment.
what does the "f" mean here?
(f"{CHECK_DIR}")
There was a problem hiding this comment.
f strings is a way to replace variables with its values : lets take on consideration this example :
VAL = 5
print(f "val = {VAL}")
the output :
val = 5
https://www.geeksforgeeks.org/formatted-string-literals-f-strings-python/
| # taking arguments from the client | ||
| args=sys.argv[1:] | ||
| if (args[0]==None): | ||
| TOOLCHAIN_INSTALL_DIR = "/usr/local/toolchains " |
There was a problem hiding this comment.
The space at the end is not needed.
| # Download and install dependency # | ||
| ################################### | ||
|
|
||
| os.chdir('c:\\tmp') |
There was a problem hiding this comment.
Same tmp problem here
tools python/setup_env.py
Outdated
| elif o == "-s": | ||
| HOME=input("Give Home Directory :") | ||
| else : | ||
| print("erreur + help function ") |
There was a problem hiding this comment.
Can you actually print a help function here? :)
There was a problem hiding this comment.
haha sure it was just a reminder print so i can write it later :D
tools python/setup_env.py
Outdated
| if(platform.release()=="Darwin"): | ||
| os.system(f"cat {HOME}/.bash_profile") | ||
| if (os.path.isfile("c:/.bash_profile")==True): | ||
| exec(open('c:/.bash_profile').read()) |
There was a problem hiding this comment.
Can you explain what this section of code is doing?
There was a problem hiding this comment.
based on the bash script
if [ "$(uname)" == "Darwin" ]; then
cat << ENDOFBLOCK >> $HOME/.bash_profile
if [ -f $HOME/.bashrc ]; then
source $HOME/.bashrc
fi
first i checked if the platform system is Mac (i need to change platform.release to platform.system) then since i didnt find any equivalent bash command " cat " in python scripts i used os.system() function
then i checked if the $HOME/.bashrc is actually a regular file and not directory
| @@ -0,0 +1,12 @@ | |||
| *.pdf filter=lfs diff=lfs merge=lfs -text | |||
There was a problem hiding this comment.
File should not be included - accidental commit, probably from running the script :).
Same is true for the .github folder, tools/.gitignore, and tools/subprojects.
2 participants
No description provided.