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

✨ Move PROS 4 to early access #296

Merged
merged 22 commits into from
Jan 18, 2024
Merged

✨ Move PROS 4 to early access #296

merged 22 commits into from
Jan 18, 2024

Conversation

12944qwerty
Copy link
Member

@12944qwerty 12944qwerty commented Sep 17, 2023

Summary:

Deprecate PROS v3.

  • Renames beta to modern or v4
  • warns user v3 is deprecated once

Closes #295

Copy link
Member

@ayushuk ayushuk left a comment

Choose a reason for hiding this comment

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

For CLI changes, we usually required you to test your changes with a test plan expected and actual outputs on the PR. Overall it looks good, good job.

pros/cli/conductor.py Outdated Show resolved Hide resolved
pros/conductor/conductor.py Outdated Show resolved Hide resolved
pros/conductor/conductor.py Outdated Show resolved Hide resolved
pros/conductor/conductor.py Outdated Show resolved Hide resolved
pros/conductor/conductor.py Outdated Show resolved Hide resolved
pros/conductor/conductor.py Outdated Show resolved Hide resolved
pros/conductor/conductor.py Outdated Show resolved Hide resolved
@12944qwerty 12944qwerty changed the title Deprecate PROS v3 🚸 "Deprecate" PROS v3 Sep 21, 2023
@ayushuk ayushuk mentioned this pull request Oct 5, 2023
@ayushuk
Copy link
Member

ayushuk commented Oct 5, 2023

Closes #295

@ayushuk ayushuk changed the title 🚸 "Deprecate" PROS v3 ✨ Move PROS 4 to early access Oct 5, 2023
@ayushuk
Copy link
Member

ayushuk commented Oct 5, 2023

Create new pros 3 project without early access flag for the first time:
new pros 3 project first time

Create a new pros 3 project after the first time without the flag:
new pros 3 project second time

Create a new pros 4 project with the early access flag:
new pros project early access flag

Create a new pros project after early access is enabled:
pros 4 project no flag

pros/cli/conductor.py Outdated Show resolved Hide resolved
pros/cli/conductor.py Outdated Show resolved Hide resolved
Co-authored-by: BennyBot <48661356+BennyBot@users.noreply.github.com>
@noam987
Copy link
Contributor

noam987 commented Oct 5, 2023

Only concern I have is the okapi Message (also We should move okapi out of being a default template)

Copy link
Member

@ayushuk ayushuk left a comment

Choose a reason for hiding this comment

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

Need to fix the okapi error

@ayushuk
Copy link
Member

ayushuk commented Nov 13, 2023

There are some issues I have with the current flow.

  1. Saying "no" to the initial prompt that says "PROS 3 is available" just raises an error and ends the execution. When you try to create a new project the directory is already created so then you try to make a project again and it gives you an error that a project already exists even though its just a directory with project.pros so then you have the delete that directory to make a project. When you make a new project after deleting the directory it makes a project in PROS 3 after the user already said no to using PROS 3. Seeing an error after saying no is probably just going to cause confusion and I think when someone says no to the prompt a PROS 4 project should just be created.
  2. There isn't any way (other than deleting conductor.pros) to turn off early access after you already enabled it. For someone that accidentally enabled early access we should have a way for them to turn off early access. This can probably be a flag. We dont want people going through hoops to reverse an action they accidentally did.
  3. I tried to make a PROS 3 project with early access enabled and I got an error where it tried to apply LVGL. The LVGL template shouldn't be trying to be applied to PROS 3. We should fix this to avoid confusion.
Screenshot 2023-11-13 at 8 59 28 AM Screenshot 2023-11-13 at 9 02 25 AM

Copy link
Member

@ayushuk ayushuk left a comment

Choose a reason for hiding this comment

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

Code lgtm, will test this out tomorrow I think we should be good to merge

@@ -91,8 +91,8 @@ def fetch(query: c.BaseTemplate):
help="Force apply the template, disregarding if the template is already installed.")
@click.option('--remove-empty-dirs/--no-remove-empty-dirs', 'remove_empty_directories', is_flag=True, default=True,
help='Remove empty directories when removing files')
@click.option('--beta', is_flag=True, default=False, show_default=True,
help='Allow applying beta templates')
@click.option('--early-access/--no-early-access', '--early/--no-early', '-ea/-nea', 'early_access', '--beta/--no-beta', default=None,
Copy link
Member

Choose a reason for hiding this comment

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

imo --disable-early-access > --no-early-access but open to more opinions

@ayushuk
Copy link
Member

ayushuk commented Dec 1, 2023

Some things after testing this.

  1. The disable early access feature works very well for me, tried some edge cases and it performs as I expect
  2. The problem I mentioned in point 1 of the large comment above about raising an error and creating a directory is still partially an issue. When you choose "no", an empty project directory is created, while you can make a new project with the same name, its still an issue to be because the directory still exists. For this reason, I think when someone selects the "no" option to "do you want to use pros 3" then you should just go ahead and make a PROS 4 project instead of having to run another command since we already know their intention.
  3. Point 3 in the large comment is still an issue

Copy link
Member

@ayushuk ayushuk left a comment

Choose a reason for hiding this comment

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

LGTM

@ayushuk ayushuk merged commit 1e32a53 into develop Jan 18, 2024
10 checks passed
@ayushuk ayushuk deleted the enhancement/rework-beta branch January 18, 2024 19:19
This was referenced Jan 19, 2024
@ayushuk ayushuk mentioned this pull request Feb 23, 2024
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.

SSL issues
4 participants