Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No errors with code, just a few potential issues #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MKM12345
Copy link

@MKM12345 MKM12345 commented Apr 25, 2023

There are no syntax errors in your original code, but there are some issues (and sugestions) that are possibly good to know:

First, the get_args() function assumes that there will always be a command line argument, but sometimes there might not be. You should add some error checking to handle this situation.

Second, using os.system() to execute command line commands isn't always safe. It's better to use the subprocess module instead, because it's more flexible and secure.

Third, the set_py_name() function assumes that Python is on the system path, but that might not always be true. You should use the sys.executable variable to get the path to the Python interpreter instead.

Fourth, your script only works on Windows because of colour 07 and django-admin.exe. It's better to use subprocess to execute platform-specific commands and make your script cross-platform.

Fifth, your script doesn't give any feedback to the user. It would be better to print output to the console so the user knows what's happening.

Finally, the script doesn't activate the virtual environment automatically, so the user has to do it manually. It's better to activate the virtual environment within the script automatically.

I think that this script should help.

Tell me if there is anything wrong.

@hamdivazim
Copy link
Owner

Ah thanks! Will merge soon! These suggestions are good

@MKM12345
Copy link
Author

You are welcome @hamdivazim!

@hamdivazim hamdivazim self-requested a review April 26, 2023 16:54
@hamdivazim hamdivazim self-assigned this Apr 26, 2023
@MKM12345
Copy link
Author

Also @hamdivazim, if any of my changes are undesired or incorrect, please tell me and I'll be happy to/for you to remove it.

@hamdivazim hamdivazim removed their request for review April 26, 2023 19:24
@hamdivazim
Copy link
Owner

Alright cool, I'll test the changes before merging

@MKM12345
Copy link
Author

Everything good?

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