-
Notifications
You must be signed in to change notification settings - Fork 35
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
Added a check for the correct robot.py capitalization. #187
Conversation
… from the correctly-cased file.
I was very behind on my commits it seems; how unfortunate. |
I'm pretty sure we already have a check for robot.py that's overridable. We need to improve that check, not break that entirely. |
The check is still present. |
Sure, but what I'm saying is that this check and that check seem to be completely separate. That breaks the intent of the override, I think. |
@benjiboy50fonz have you actually tested this PR? As @auscompgeek pointed out I don't think this is doing what you intended. |
Ok, I understand @auscompgeek . @virtuald, I haven't had access to my RIO yet, but I will soon. I'd set up a VM, but I'm on Windows and WSL is a pain, as well as a VM in a VM. |
It's generally a bad idea to make changes to code without testing those changes. |
... so what's going on with this? |
This is a fine idea, but it needs to actually work if we're going to accept it. |
In response to robotpy/robotpy-installer#110, I propose a solution.
Because
os.listdir
offers a list of directory contents, and we have a specific file we want to search for, I simply added a check to see ifrobot.py
was in the directory which called the deploy command. If it's not, it assumes incorrect capitalization and fails.I did it this way as well to prevent programmers from intentionally naming their file "Robot.py", and then encountering issues down the road. Hopefully this cuts them short, and gives them enough clarity that they can modify their codebase to meet this naming standard.
Criticism is very welcome!