[#1715] Add RepoSense CLI Wizard Walkthrough #1950

sikai00 · 2023-03-15T16:45:08Z

Fixes #1715
This PR is co-authored with @MarcusTXK.

Proposed commit message

For a new user trying to get into RepoSense, the command line arguments
may be hard and intimidating to navigate and takes some effort to 
reference from the user guide. If a new user wants to try out RepoSense
(instead of immediately putting it to actual use), he may be put off 
by the effort needed for entry.

Let's implement a RepoSense CLI wizard. This makes it easier for a 
new user to understand how a run of RepoSense may work, understand 
some of the flags we have, and eases the learning curve required 
to use RepoSense.

Co-authored-by: Marcus Tang <[email protected]>
Co-authored-by: Si Kai <[email protected]>

Other information

How to test

Pull from the PR branch and run reposense with the flag --init.

Overview of the wizard implementation

The wizard is implemented using a discrete event simulator (DES) style. This allows us to avoid long if-else conditions for the prompting logic, as each prompt can invoke another prompt simply by creating it and adding it into the queue.

└── wizard/
    ├── prompts/
    │   ├── Prompt.java
    │   ├── OptionalPrompt.java
    │   └── ...
    ├── WizardRunner.java
    ├── Wizard.java
    ├── BasicWizard.java
    └── InputBuilder.java

The WizardRunner.java acts as the simulator and takes in Wizard.java (implemented under BasicWizard.java, so as to allow for different future Wizards). WizardRunner.java maintains a Deque of Prompt, which is initially supplemented by a concrete implementation of Wizard.java.

Each Prompt.java may be used to add a config flag and its argument via InputBuilder.java by calling WizardRunner::buildInput(Scanner sc). At the end when no other prompts exist in the deque, the method ends. The complete input string is created when WizardRunner::getBuiltInput() and subsequently used when WizardRunner::run() is finally called. In a similar sense to that of the current systemtest, the string is directly passed into RepoSense::main to produce the expected output and behavior.

Each concrete implementation of Prompt.java can return even more prompts to the Deque, so a Prompt is able to define the next Prompt that should be run. This is particularly useful in this example case where we ask the user a yes-or-no question if they want to indicate the start date for analysis, we can follow that up with a Prompt to get the date or choose to simply move on.

OptionalPrompt

OptionalPrompt is an abstract subclass of Prompt, as there were many Prompt that required a yes-or-no-to-have question, then requiring a follow-up Prompt should the user answer yes. The follow-up is provided through the optionallyRun method defined only in OptionalPrompt, such that the WizardRunner sees no difference between an OptionalPrompt and a regular Prompt.

Possible future improvements

Below are some possible future improvements that were not included in the interest of time and to ensure a minimal viable product.

Currently, errors are only detected when we pass the completed input string to RepoSense::main. If a user were to mistype an input, they will have to go through the trouble of re-entering all the inputs. We can have better error prompts such that in the case of an invalid input, we can immediately correct and re-prompt, avoiding having to make the user go through the whole setup again just to correct an erroneous input.

Add DES skeleton code

Add cli wizard flag and optional, view and repo prompts

Add UntilPrompt

Add documentation for init command Co-authored-by: Marcus Tang <[email protected]> Co-authored-by: Si Kai <[email protected]>

chan-j-d · 2023-03-31T20:25:43Z

Some quick comments before reviewing:

Should probably add the command to test somewhere in the PR itself
The commit message's final line should be a short call to action. Something like 'Let's implement a setup walkthrough wizard'. The additional reasoning can be moved into the explanation portion of the commit message

chan-j-d

Some quick comments. I think the code is overall pretty well written and don't have too much to say in that direction. And regarding testing I'm also not too sure how much automated testing can be done

chan-j-d · 2023-03-31T20:44:32Z

src/main/java/reposense/wizard/InputBuilder.java

+     *
+     * @param paths The repo paths.
+     */
+    public InputBuilder addRepos(String paths) {


Does this work well with multiple quoted file path arguments that contain spaces?

Yup, it simply passes the input entirely to RepoSense::main, where it is parsed as if the user passed it via the cli, so all parsing functionality for file paths will be the same.

chan-j-d · 2023-03-31T20:46:00Z

src/main/java/reposense/wizard/InputBuilder.java

+
+    public boolean isShallowCloning() {
+        return shallowCloning;
+    }


I think we should only add the ones we are currently using into this class and delete the remaining. I don't think this feature will grow to implement many of the other flags

Got it, we will clean up the unused InputBuilder.java functions.

chan-j-d · 2023-03-31T20:46:50Z

src/main/java/reposense/wizard/prompt/ViewPrompt.java

+    @Override
+    public Prompt[] run() {
+        return new Prompt[] {};
+    }


I don't think this is correct. It should be view or no view without specification. The -v with argument is for when the user does not want to analyze anything and just wants to view a pre-analyzed report

The logic for this is implemented in OptionalViewPrompt.java, where the user is prompted with "Do you want to start a server to display the report?", allowing them to choose whether or not to use the ViewPrompt. ViewPrompt currently contains logic for configuration of the --view flag if it is present.

I understand that. But the OptionalViewPrompt shouldn't spawn a ViewPrompt since they dont serve the same purpose. There is no need for the user to supply an argument for --view unless they simply want to view a report that has already been generated, so it doesn't really make sense at all in this wizard walkthrough

Yes, for this I believe it will be better to drop the need for an argument as there is no way of changing the output directory immediately after generating it anyway.

chan-j-d · 2023-03-31T20:48:04Z

src/main/java/reposense/wizard/BasicWizard.java

+            new OptionalUntilPrompt(),
+            new OptionalViewPrompt(),
+            new RepoPrompt()
+    };


I tried it myself and I don't think it makes a lot of sense to put the repo last. I think it should be first since I'm usually thinking of what I want to analyze and then the time interval to analyze rather than the other way around

I agree, lets change it to be the first.

…ilder, Update BasicWizard order

…0/RepoSense into 1715-reposense-cli-wizard

chan-j-d

Documentation changes look good. I made quite a few comments and suggestions this time round after reviewing it again since I understand the PR better now.

Also wanted to know if you thought it might be better to include some fancy text like [RepoSense Wizard]: to kind of denote that the program is 'talking to the user'.

src/main/java/reposense/wizard/InputBuilder.java

src/main/java/reposense/wizard/prompt/Prompt.java

chan-j-d · 2023-04-05T08:11:09Z

src/main/java/reposense/wizard/prompt/OptionalPrompt.java

+        // Repeat OptionalPrompt until a valid input is provided
+        return new Prompt[] {this};
+    }
+}


Not a requested change but more a point of discussion. Was wondering if there is a point in creating the subclasses since you could get this class to have a constructor that takes in the description and an instance of the prompt to be shown if Yes is chosen.

I wrote this comment more from the perspective of testing since the subclass behaviors are almost identical I feel. See other comment about testing

In what you mentioned, do you mean to have a single class per flag (e.g., SincePrompt should handle both asking if it should take in a SincePrompt, as well as actually taking in a SincePrompt)?

In a previous implementation, it actually took in a Prompt in the constructor so we could avoid spawning optional subclasses. However, we decided that this design wass a little strange OOP-wise.

You can view the discussion me and @sikai00 had here.

I was referring to the version Marcus mentioned. I don't think cyclic dependency is correct for it, the dependency relations are almost the same in either case and is not cyclic since Prompt doesnt depend on OptionalPrompt. This just changes a dependency to an association. Regardless, I don't think it's a big issue

chan-j-d · 2023-04-05T08:49:07Z

src/test/java/reposense/wizard/WizardRunnerTest.java

+                .build();
+        assertEquals(expectedInput, wizardRunner.getBuiltInput());
+    }
+}


I was wondering if it might be better to test individual prompts instead and have a separate test suite for input builder which tests combining multiple repo flags.
This way, you don't have to rewrite the inputs every time the ordering of prompts is changed.

Additionally, I think it's missing test cases for completely invalid inputs for the optional prompt queries since those are supposed to repeat on invalid inputs if I'm understanding it correctly.

We can implement a TestWizard (similar to BasicWizard) that takes in individual prompts. That way, we can test any individual prompts/combinations of prompts. In this way, we reduce the number of test cases for BasicWizard as such, so that the re-writing of input doesn't have to happen for any of the possible new wizards we write, while still being able to test for correctness overall.

Just to check, the test case you mentioned is the one where we repeated input garbage to the OptionalPrompt, and finally a "yes"?

Sorry I'm not sure if I'm misunderstanding this

Just to check, the test case you mentioned is the one where we repeated input garbage to the OptionalPrompt, and finally a "yes"?

But is that suggesting that such a test case exists already or that that is the test case I want. Because I don't think I see such a test case and yes that's kind of what I want.

In general, I'd like to see more individualized test cases and only a few combined ones that are preferably done in a way that is independent of the order of the actual prompts since those may be reordered arbitrarily and we wouldnt want to have to fix test cases every time we do that.

Also I guess I have to ask at this point what the difference between TestWizard and Wizard would be

But is that suggesting that such a test case exists already or that that is the test case I want. Because I don't think I see such a test case and yes that's kind of what I want.

Sorry for the confusion, this is suggesting the test case that you want, and does not already exist.

In general, I'd like to see more individualized test cases and only a few combined ones that are preferably done in a way that is independent of the order of the actual prompts since those may be reordered arbitrarily and we wouldnt want to have to fix test cases every time we do that.

This would be a beneficial change. Though it is as you mentioned, there is still going to be some (but minimal number) of test cases with the ordered prompts.

Also I guess I have to ask at this point what the difference between TestWizard and Wizard would be

TestWizard would just be a concrete implementation of the abstract Wizard class. For example, we have a BasicWizard currently. TestWizard will be a new, test-specific implementation of the abstract Wizard class where we can input specific types of Prompts specifically to test the prompts within the TestWizard.

chan-j-d · 2023-04-05T08:50:16Z

src/main/java/reposense/wizard/InputBuilder.java

+        return addQuotationMarksToPath(path.toString());
+    }
+
+}


Actually, I recall that there is an InputBuilder class in a util package in test. Was wondering if it might be worthwhile to move that class over (apologies about this in relation to my previous comment about unused input flags)

github-actions · 2023-05-13T00:15:01Z

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

github-actions · 2023-05-21T00:17:12Z

This PR was closed because it has been marked as stale for 7 days with no activity.
Feel free to reopen this PR if you would like to continue working on it.

sikai00 and others added 22 commits March 5, 2023 12:46

Add DES skeleton code

56b693a

Add docs

306d157

Fix import groups in test

a26facc

Fix prompt adding ordering

534ed26

Merge pull request #1 from sikai00/1715-add-skeleton

845abe3

Add DES skeleton code

Add wizard cli flag to ArgsParser

bcf29ca

Add view prompt and optional prompt

96c24c6

Add repo prompt and update since prompt to be optinal

9b39e50

Fix java.util.NoSuchElementException when running wizard via gradle

d30c0e8

Fix import wildcard

a713677

Refactor optinal prompt and Update prompt field names

9a62d24

Update optional prompt

181a62c

Fix checkstyle error

15577ae

Update WizardRunnerTest

e83fbf0

Update wizard input builder

ac8fd12

Merge pull request #2 from MarcusTXK/1715-reposense-cli-wizard

ee5e006

Add cli wizard flag and optional, view and repo prompts

Merge branch 'master' into 1715-reposense-cli-wizard

f80e0a5

Add UntilPrompt

8b1b7b4

Merge pull request #3 from sikai00/1715-add-untilprompt

8cd9366

Add UntilPrompt

Add documentaion for init

9b47b33

Reorganise file structure

8eb642d

Rename prompts to prompt

e006376

sikai00 assigned MarcusTXK and sikai00 Mar 15, 2023

github-actions bot requested a deployment to dashboard-1950 March 15, 2023 16:45 Abandoned

github-actions bot requested a deployment to docs-1950 March 15, 2023 16:45 Abandoned

sikai00 unassigned MarcusTXK and sikai00 Mar 15, 2023

MarcusTXK and others added 2 commits March 16, 2023 01:08

Update cli.md

4cadcca

Merge pull request #4 from sikai00/1715-add-documentation

33cee39

Add documentation for init command Co-authored-by: Marcus Tang <[email protected]> Co-authored-by: Si Kai <[email protected]>

chan-j-d requested changes Mar 31, 2023

View reviewed changes

Fix addRepos missing whitespace bug, Remove unused methods in inputBu…

60563c0

…ilder, Update BasicWizard order

github-actions bot requested a deployment to dashboard-1950 April 2, 2023 14:01 Abandoned

github-actions bot requested a deployment to docs-1950 April 2, 2023 14:01 Abandoned

Fix failing WizardRunner tests

41e7669

github-actions bot requested a deployment to dashboard-1950 April 2, 2023 14:07 Abandoned

github-actions bot requested a deployment to docs-1950 April 2, 2023 14:07 Abandoned

Merge branch 'master' into 1715-reposense-cli-wizard

561f60d

github-actions bot temporarily deployed to dashboard-1950 April 2, 2023 14:08 Inactive

github-actions bot temporarily deployed to docs-1950 April 2, 2023 14:08 Inactive

Add docs to using the init flag

f2815d6

github-actions bot deployed to dashboard-1950 April 3, 2023 12:13 View deployment

github-actions bot deployed to docs-1950 April 3, 2023 12:13 View deployment

MarcusTXK added 2 commits April 4, 2023 01:02

Update ViewPrompt

9c3f6c2

Merge branch '1715-reposense-cli-wizard' of https://github.com/sikai0…

61e7195

…0/RepoSense into 1715-reposense-cli-wizard

github-actions bot requested a deployment to dashboard-1950 April 3, 2023 17:03 Abandoned

github-actions bot requested a deployment to docs-1950 April 3, 2023 17:03 Abandoned

MarcusTXK requested a review from chan-j-d April 4, 2023 15:07

chan-j-d requested changes Apr 5, 2023

View reviewed changes

github-actions bot added the Stale label May 13, 2023

github-actions bot closed this May 21, 2023

ckcherry23 reopened this Mar 9, 2024

github-actions bot requested a deployment to dashboard-1950 March 9, 2024 14:34 Abandoned

github-actions bot requested a deployment to docs-1950 March 9, 2024 14:34 Abandoned

ckcherry23 added s.DoNotMerge and removed Stale labels Mar 9, 2024

MarcusTXK removed the s.ToReview label Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1715] Add RepoSense CLI Wizard Walkthrough #1950

[#1715] Add RepoSense CLI Wizard Walkthrough #1950

sikai00 commented Mar 15, 2023 •

edited by chan-j-d

Loading

chan-j-d commented Mar 31, 2023

chan-j-d left a comment

chan-j-d Mar 31, 2023

MarcusTXK Apr 2, 2023

chan-j-d Mar 31, 2023

MarcusTXK Apr 2, 2023

chan-j-d Mar 31, 2023

MarcusTXK Apr 2, 2023

chan-j-d Apr 2, 2023

sikai00 Apr 3, 2023

chan-j-d Mar 31, 2023

MarcusTXK Apr 2, 2023

chan-j-d left a comment

chan-j-d Apr 5, 2023

sikai00 Apr 10, 2023

MarcusTXK Apr 10, 2023

chan-j-d Apr 10, 2023

chan-j-d Apr 5, 2023

sikai00 Apr 10, 2023

chan-j-d Apr 10, 2023

sikai00 Apr 12, 2023

chan-j-d Apr 5, 2023

github-actions bot commented May 13, 2023

github-actions bot commented May 21, 2023

[#1715] Add RepoSense CLI Wizard Walkthrough #1950

Are you sure you want to change the base?

[#1715] Add RepoSense CLI Wizard Walkthrough #1950

Conversation

sikai00 commented Mar 15, 2023 • edited by chan-j-d Loading

Proposed commit message

Other information

How to test

Overview of the wizard implementation

OptionalPrompt

Possible future improvements

chan-j-d commented Mar 31, 2023

chan-j-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chan-j-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2023

github-actions bot commented May 21, 2023

sikai00 commented Mar 15, 2023 •

edited by chan-j-d

Loading