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

Added a check for the correct robot.py capitalization. #187

Closed
wants to merge 18 commits into from

Conversation

benjiboy50fonz
Copy link
Contributor

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 if robot.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!

@benjiboy50fonz
Copy link
Contributor Author

I was very behind on my commits it seems; how unfortunate.

@auscompgeek
Copy link
Member

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.

@benjiboy50fonz
Copy link
Contributor Author

The check is still present.

@auscompgeek
Copy link
Member

auscompgeek commented Aug 1, 2021

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.

@virtuald
Copy link
Member

virtuald commented Aug 1, 2021

@benjiboy50fonz have you actually tested this PR? As @auscompgeek pointed out I don't think this is doing what you intended.

@benjiboy50fonz
Copy link
Contributor Author

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.

@virtuald
Copy link
Member

virtuald commented Aug 2, 2021

It's generally a bad idea to make changes to code without testing those changes.

@virtuald
Copy link
Member

... so what's going on with this?

@virtuald virtuald closed this Jan 30, 2022
@virtuald
Copy link
Member

This is a fine idea, but it needs to actually work if we're going to accept it.

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.

3 participants