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

🐛Fixes --run-after flag priority issues. #173

Merged
merged 7 commits into from
Oct 29, 2021

Conversation

BennyBot
Copy link
Member

Summary:

Changes logic statements when deciding which vex.V5Device.FTCompleteOptions argument to pass. Essentially gives the --run-after switch priority over the --run-screen switch in the if/elif/else chain.

Motivation:

Quality of life feature for users that will allow them to make changes and test them more efficiently. Removes the need to go through the controller menu or press buttons on the brain to test changes.

References (optional):

closes #132

Test Plan:

Tested every combination of --run-after and --run-screen switches to verify that new logic works.
The link to a video of the test cases is here: https://drive.google.com/file/d/1mYM6LzFtDHzZ1Jm6cl8S6FvG-k9EKSDa/view?usp=sharing

Copy link
Member

@WillXuCodes WillXuCodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one thing we should do for this, is to add some aliases for some of these options (such as -r for running after uploading, or something along the lines of that). I'm honestly not sure what the best abbreviations would be so anybody can feel free to chime in on this.

@HotelCalifornia
Copy link
Contributor

I'd prefer you didn't modify all the flags in this PR... imo the other aliases should go in a separate one

@HotelCalifornia
Copy link
Contributor

I also think having both run after and run screen is confusing because they're mutually exclusive in some cases.

run_screen run_after behaviour
T T unclear to user
T F run program post upload
F T show run screen post upload
F F show nothing on the brain?

maybe it would be a cleaner solution to fold these two flag options into one enum option action, so that the user can specify which of the three valid actions they want

$ pros upload --action=RUN
# run program after uploading

$ pros upload --action=SCREEN
# show screen after uploading (default)

$ pros upload --action=NOTHING
# show nothing and execute nothing after uploading

@BennyBot
Copy link
Member Author

The most recent commit adds a class for deprecated options. Can be used for any option that is deprecated by adding/replacing @click.option(cls=PROSDeprecated). This class has an optional argument for the option it was replaced with. Example: @click.option(cls=PROSDeprecated,replacement='new-option'). This class was used to deprecate the '--run-after' and '--run-screen' flags. Adds '--after' switch with options 'run', 'screen', and 'none'. New logic to use '--after' switch primarily, but still support '--run-after' and '--run-screen' flags if they are used. Adds aliases for '--run-after', '--run-screen', and '--after'. These aliases are currently '-ra/-nr', '-rs/-ex', and '-af'. These are subject to change if anyone has better aliases for these switches. It is also probably a better idea to not add aliases for the '--run-after' and '--run-screen' flags because they are deprecated.

@BennyBot BennyBot requested a review from WillXuCodes October 22, 2021 01:20
@kunwarsahni01 kunwarsahni01 self-requested a review October 29, 2021 12:43
Copy link
Member

@kunwarsahni01 kunwarsahni01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, was functioning properly in testing.

@kunwarsahni01 kunwarsahni01 merged commit 551e793 into develop Oct 29, 2021
@WillXuCodes WillXuCodes deleted the bugfix/run-after-useless branch October 29, 2021 12:59
@kunwarsahni01 kunwarsahni01 mentioned this pull request Nov 19, 2021
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.

Upload's run-after switch does nothing
4 participants