Refactor parser #39

JoeyChenSmart · 2020-09-27T13:45:15Z

Closes #33

Much cleaner/usable/pleasant API. Do let me know if anything here is unclear.

Removed many tests that were irrelevant (addressbook-specific), replaced them with a short comment summary of what the test was trying to achieve.

New API

Brief idea

Ideally, the new Commands should be thought of as 'generalized functions': You only need to specify as private variables
what parameters the Command takes in, and how the Command should use these parameters to produce changes in the Model and output a CommandResult.

Example

public class AddCommand extends Command {
    public static final String MESSAGE_SUCCESS = "New person added: %1$s";
    public static final String MESSAGE_DUPLICATE_PERSON = "This person already exists in the address book";

    private final Parameter<Name> nameParameter = this.addParameter(
        "name",                           // The name for this parameter
        "n",                              // The flag for this parameter, i.e. n for '-n' in the command 'add -n John Doe ...'
        "Name of person to add",          // Short description of the value of this parameter. Used in auto-generating help
        "John Doe",                       // Example of the value of this parameter. Used in auto-generating help
        ParserUtil::parseName             // A function that converts a String to a Name object for use later
    );
    private final Parameter<Phone> phoneParameter = <REDACTED>
    private final Parameter<Email> emailParameter = <REDACTED>

    @Override
    public CommandResult execute(Model model) throws CommandException {
        requireNonNull(model);
        // rewriting this class as an example, address and tags not implemented.
        Name newName = nameParameter.consume(); // Parameter#consume() will return the Name parsed using ParserUtil::parseName
        Phone newPhone = phoneParameter.consume();
        Person toAdd = new Person(newName, newPhone, emailParameter.consume(),
            new Address("dummy value"), new HashSet<>());

        if (model.hasPerson(toAdd)) {
            throw new CommandException(MESSAGE_DUPLICATE_PERSON);
        }

        model.addPerson(toAdd);
        return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd));
    }
}

In the code, first we declare some Parameters. These parameters will be used in the parsing. Their parsed value can be
retrieved later in the execute function using the Parameter<T>#consume() function. This function will return the value
as a type T object. Note how each declaration of Parameter also serves as immediate documentation for the command.

Each parameter will take in every character as a string until the next parameter is encountered. E.g. in
add -n John Doe Donald -p 1234
the -n parameter will recieve "John Doe Donald"

Note that there is no constructor. All variables to be used should either be globals or supplied as Parameters.

The execute function is the same as before, and will contain the main logic to be executed. The only difference is that we now
access variables using Parameter<T>#consume() instead of accessing variables stored by the constructor.

Finally, the command will be 'hooked' into the McGymmyParser

private void addDefaultCommands() {
        this.addCommand("add", AddCommand::new);
        this.addCommand("edit", EditCommand::new);
        this.addCommand("delete", DeleteCommand::new);
        ... <REDACTED>
    }

The first argument "add" is the 'command word' to invoke the command, and the second argument is the command's constructor. Note that this does not have to be a constructor, just a function that takes in no arguments and returns a new Command.

There are more examples of Commands you can find in the source code. In particular the changed classes are the EditCommand, DeleteCommand and FindCommand.

Just a few things left to note:

Optional Parameters

Some parameters may be declared as optional, e.g.

// from EditCommand.java
private final OptionalParameter<Name> nameParameter = this.addOptionalParameter(
        "name",
        "n",
        "Name of person to add",
        "John Doe",
        ParserUtil::parseName
    );

The arguments are exactly the same as its Parameter counterpart. These parameters may be omitted by the user when entering the command. The difference is that instead of using Parameter<T>#consume() we have a new function OptionalParameter<T>#getValue() that returns a Optional<T> value. If the user supplied this parameter, it will be Optional.of the parsed value, otherwise it will be Optional.empty.

Unnamed Parameters

Some parameters do not have flags, e.g. 1 in delete 1.
To create a parameter that does this, simply leave the flag as an empty string, i.e.

private final Parameter<Index> indexParameter = this.addParameter(
        "index",
        "",
        "index number used in the displayed person list.",
        "2",
        ParserUtil::parseIndex
    );

The parameter will work exactly the same way as before, and you can retrieve its value normally using Parameter<T>#consume().

Can we declare multiple of the same parameters? (e.g. 2 unnamed parameters)

Nope, if you try to declare multiple of the same parameters there will be an assertion error somewhere.

Technically you can work around this, by declaring a new class that contains the two parameters, e.g.
the command:
takeTwoIntegers 1 2
may have a parameter
private Parameter<IntPair> param = this.addParameter(..., IntPair::parse)

and the IntPair class is as follows:

class IntPair {
  public int a;
  public int b;
  public parse(String str) {
    IntPair result = new IntPair();
    result.a = ...
  }

Don't think any of our commands will involve these kinds of parameters, but if they do we may have to either redesign the command or the parser. (We'll solve that problem when it happens)

Does the position of the unnamed parameter matter?

For example, is there a difference between
edit 1 -n John Doe
and
edit -n John Doe 1

Yes, recall that each parameter will take in all the subsequent characters until the next parameter as a string.

For the first example the 1 will be considered as input to the -n parameter, and the unnamed parameter will have no input (so there will be an error).

Motivation

Here I will try to detail some concrete benefits gained from this change.

Most of the changes here are inspired by Django. For example creating forms in Django is very declarative and I think that is a better way to create code.

Remove the need for classes like EditPersonDescriptor

There are many things imo that are wrong philosophically with the EditPersonDescriptor class, but I'll limit what
I say here to objective improvements from removing it.

Having less code will make the codebase easier to understand and speed up development.
Creating an extra class like this for no reason will give us more surface area for bugs to fester. Writing tests
for these classes also contribute significantly to the codebase (see point 1)
The class in is just very unintuitive (why do I have to create a new object just to edit an existing object that is very simple?).

The Parser seemed like it was doing most of the work in the old architecture.

E.g. the AddCommandParser was doing most of the work creating the Person object, and the AddCommand
simply put the new object in the model. Granted, this technically fits the definition of a 'parser' but now the
idea is that the parser should create objects at such a high level of abstraction, and whatever work that is left
for the Command. Also, now that the parser produces objects at a high level of abstraction, we have to do all
sorts of OOP gymnastics (like creating an EditPersonDescriptor) to get simple jobs done.

Using a library reduces the amount we need to implement/test. Also gives us features for free.

The library also helps us automatically generate USAGE strings for each commands. Isn't that just great?

Parsers are now 'automatically generated'

The logic of parsing is abstracted to creating a Parameter. Parsing each parameter is also very modular,
as we only need a String -> T function to parse each parameter.

Also now that we don't need to write individual parsers, we have less things to test.

But won't this make the Parser and Command tightly coupled?

In our use case they're inherently tightly coupled, so separating them will bring more harm than good.

Either way they were already quite tightly coupled in the first place.

Internals/Implementation

Here I will give a brief description of how this works internally. Will add more details if needed.

Each Parameter is simply a container, which can hold a T typed variable, and a function to convert a String to a T.

Command's this.addParameter is a protected function that creates the Parameter object and stores it in a ParameterSet.

Parsing is a 2-step process.

The parser reads in the first word in the user's raw input string, and selects the creates a new instance of the corresponding Command.
The parser pulls out the Command's ParameterSet and gives passes the raw input parameters as strings to each Parameter. Each parameter will then store the T type variable produced from the converter function. These parameters are now ready to use in the Command's execute function. The parser returns the ready-to-use Command.

Enhancements

A few possible enhancements that can be implemented in the future

1. Implement CommandToFunction helper method

Allow a Command to be executed in another command as a void function.
e.g. in EditCommand we may want to run AddCommand.asFunction(model, name, phone, email, ...) to add the edited person.

Sounds useful, not sure if it will be in practice

2. Create Parameter#toOptionalParameter methods

Create a method to easily convert a Parameter into an OptionalParameter and vice versa

into build.gradle

Added skeleton for parameter classes, and some convenience function in command.java. Also copied over functions from iP and added some basic functionality. TODO: JavaDoc TODO: implement parser TODO: replace old commands TODO: add/change tests

Add exceptions and tests for parameter related classes.

mainly NPE. Code works now

Deleted most of the old parser code and the related tests. Did not delete all the test files, but instead replaced them with a todo containing what was being tested before.

also deleted many irrelevant tests.

aidoxe-123

Overall, this new parser idea and implementation looks good to me. However, I think it would be better to add more test cases to make sure that this parser works well with the address book first, since we know the old parser's behavior in address book. After making sure that the parser works fine, we can adopt it to McGymmy later.

aidoxe-123 · 2020-09-28T15:57:19Z

src/test/java/seedu/address/logic/parser/ParserUtilTest.java

@@ -1,196 +0,0 @@
-package seedu.address.logic.parser;


Since ParserUtil.java is still there, is it better to keep this test file instead of deleting it?

aidoxe-123 · 2020-09-28T16:02:47Z

src/test/java/seedu/address/logic/LogicManagerTest.java

@@ -68,26 +60,6 @@ public void execute_validCommand_success() throws Exception {
        assertCommandSuccess(listCommand, ListCommand.MESSAGE_SUCCESS, model);
    }

-    @Test


What's wrong with this test

Hmm looks like I may have accidentally removed this during the safe delete or something, will update and restore it.

aidoxe-123 · 2020-09-28T16:15:43Z

src/main/java/seedu/address/logic/commands/EditCommand.java

-        this.index = index;
-        this.editPersonDescriptor = new EditPersonDescriptor(editPersonDescriptor);
-    }
+    private final Parameter<Index> indexParameter = this.addParameter(


Does the position of the unnamed parameter matter?

How does the program behave towards these commands:
edit -n JohnDoe 1
edit1 -n JohnDoe

Yes, for the first one the 1 will be considered as input to the -n parameter, and the unnamed parameter will have no input (so there will be an error).

I'll add this and the comment below into the PR's docs.

aidoxe-123 · 2020-09-28T16:16:51Z

src/main/java/seedu/address/logic/commands/EditCommand.java

-        this.index = index;
-        this.editPersonDescriptor = new EditPersonDescriptor(editPersonDescriptor);
-    }
+    private final Parameter<Index> indexParameter = this.addParameter(


Just to justify my curiosity, can a command have multiple unnamed parameters?

Nope, if you try to declare multiple of the same parameters there will be an assertion error somewhere.

Technically you can work around this, by declaring a new class that contains the two parameters, e.g.
the command:
takeTwoIntegers 1 2
may have a parameter
private Parameter<IntPair> param = this.addParameter(..., IntPair::parse)

and the IntPair class is as follows:

class IntPair { public int a; public int b; public parse(String str) { IntPair result = new IntPair(); result.a = ... }

Don't think any of our commands will involve these kinds of parameters, but if they do we may have to either redesign the command or the parser. (We'll solve that problem when it happens)

aidoxe-123 · 2020-09-28T16:39:31Z

src/main/java/seedu/address/logic/parser/McGymmyParser.java

+        this.addCommand("add", AddCommand::new);
+        this.addCommand("edit", EditCommand::new);
+        this.addCommand("delete", DeleteCommand::new);
+        this.addCommand("clear", ClearCommand::new);
+        this.addCommand("exit", ExitCommand::new);
+        this.addCommand("find", FindCommand::new);
+        this.addCommand("list", ListCommand::new);
+        this.addCommand("help", HelpCommand::new);


I think this should be AddCommand.COMMAND_WORD instead of "add", the same for other commands

Yes that seems to be better. Good catch.

aidoxe-123 · 2020-09-28T16:55:31Z

src/test/java/seedu/address/logic/commands/CommandTestUtil.java

@@ -2,11 +2,6 @@



Same comment as AddCommandTest.java

aidoxe-123 · 2020-09-28T16:55:59Z

src/test/java/seedu/address/logic/commands/DeleteCommandTest.java

@@ -1,109 +1,13 @@
 package seedu.address.logic.commands;


Same comment as AddCommandTest.java

aidoxe-123 · 2020-09-28T16:56:13Z

src/test/java/seedu/address/logic/commands/EditCommandTest.java

@@ -1,173 +1,17 @@
 package seedu.address.logic.commands;


Same comment as AddCommandTest.java

aidoxe-123 · 2020-09-28T16:56:25Z

src/test/java/seedu/address/logic/commands/FindCommandTest.java

@@ -1,83 +1,12 @@
 package seedu.address.logic.commands;


Same comment as AddCommandTest.java

aidoxe-123 · 2020-09-28T16:59:41Z

src/test/java/seedu/address/logic/parser/McGymmyParserTest.java

@@ -0,0 +1,98 @@
+package seedu.address.logic.parser;


Maybe add some more tests for the old address book's logic, to make sure that it works exactly as expected as the old parser?

I'll add a couple more tests for this, along with some unit/integration tests with the Commands.

Jh123x

LGTM
Might need a little more test cases and need to add back tags later

To facilitate testing. Package private access.

Instead of magic strings.

Mainly integration tests with the commands. Deleted AddCommandIntegrationTest which is a copy of AddCommandTest.

Jh123x

Should be able to merge soon >.<

Jh123x · 2020-09-30T03:07:59Z

src/test/java/seedu/address/logic/LogicManagerTest.java

-        String addCommand = AddCommand.COMMAND_WORD + NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY
-                + ADDRESS_DESC_AMY;
-        Person expectedPerson = new PersonBuilder(AMY).withTags().build();
+        String addCommand = AddCommand.COMMAND_WORD + " -n amy -p 99999999 -e amy@amy.com";


Might want to change this to a constant?
Like VALID_NAME_AMY, similar to the one u did in edit command test

No point imo since I'm not reusing the string outside this function.
Also would add more noise which we're gonna change anyway (when we change people to food).

Ookie in that case LTGM

Jh123x

LGTM

Jh123x · 2020-10-02T09:15:35Z

Tags and Addr not implemented as they will be removed by the end of the iteration

JoeyChenSmart added 7 commits September 25, 2020 21:51

Add apache commons-cli dependency

bd10112

into build.gradle

Add parameter classes

14a3c3c

Added skeleton for parameter classes, and some convenience function in command.java. Also copied over functions from iP and added some basic functionality. TODO: JavaDoc TODO: implement parser TODO: replace old commands TODO: add/change tests

Add exceptions and tests

eae0cca

Add exceptions and tests for parameter related classes.

Fix copious bugs

2028961

mainly NPE. Code works now

Big cleanup

362cdaa

Deleted most of the old parser code and the related tests. Did not delete all the test files, but instead replaced them with a todo containing what was being tested before.

Add tests

707d44d

also deleted many irrelevant tests.

Add docs and fix checkstyle

7b90933

JoeyChenSmart added the type.Enhancement New feature or request (!DELETE ME IF DASHBOARD IS NOT UPDATING) label Sep 27, 2020

Run dos2unix

c952942

JoeyChenSmart changed the title ~~[WIP] Refactor parser~~ Refactor parser Sep 28, 2020

aidoxe-123 reviewed Sep 28, 2020

View reviewed changes

Jh123x reviewed Sep 28, 2020

View reviewed changes

chewypiano added this to the v1.2 milestone Sep 29, 2020

Jh123x assigned JoeyChenSmart Sep 29, 2020

JoeyChenSmart added 7 commits September 29, 2020 16:31

Restore ParserUtilTest

1918917

Add setParameters methods to commands

c170030

To facilitate testing. Package private access.

Use Command Word in McGymmyParser

d47c7e7

Instead of magic strings.

Add command tests

62c5105

Add more tests to parser

056be7d

Add more tests for parser

5138dd5

Mainly integration tests with the commands. Deleted AddCommandIntegrationTest which is a copy of AddCommandTest.

Fix formatting issues

c27975a

Jh123x reviewed Sep 30, 2020

View reviewed changes

Improve error messages to user

1c56b2e

Jh123x approved these changes Oct 2, 2020

View reviewed changes

Jh123x merged commit 881dea8 into AY2021S1-CS2103T-W17-3:master Oct 2, 2020

JoeyChenSmart mentioned this pull request Oct 20, 2020

Update logic writeup #101

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor parser #39

Refactor parser #39

JoeyChenSmart commented Sep 27, 2020 •

edited

aidoxe-123 left a comment

aidoxe-123 Sep 28, 2020

aidoxe-123 Sep 28, 2020

JoeyChenSmart Sep 29, 2020

aidoxe-123 Sep 28, 2020

JoeyChenSmart Sep 29, 2020

aidoxe-123 Sep 28, 2020

JoeyChenSmart Sep 29, 2020 •

edited

aidoxe-123 Sep 28, 2020

JoeyChenSmart Sep 29, 2020

aidoxe-123 Sep 28, 2020

aidoxe-123 Sep 28, 2020

aidoxe-123 Sep 28, 2020

aidoxe-123 Sep 28, 2020

aidoxe-123 Sep 28, 2020

JoeyChenSmart Sep 29, 2020

Jh123x left a comment •

edited

Jh123x left a comment

Jh123x Sep 30, 2020

JoeyChenSmart Sep 30, 2020

Jh123x Sep 30, 2020

Jh123x left a comment

Jh123x commented Oct 2, 2020

Refactor parser #39

Refactor parser #39

Conversation

JoeyChenSmart commented Sep 27, 2020 • edited

New API

Brief idea

Example

Optional Parameters

Unnamed Parameters

Can we declare multiple of the same parameters? (e.g. 2 unnamed parameters)

Does the position of the unnamed parameter matter?

Motivation

Remove the need for classes like EditPersonDescriptor

The Parser seemed like it was doing most of the work in the old architecture.

Using a library reduces the amount we need to implement/test. Also gives us features for free.

Parsers are now 'automatically generated'

But won't this make the Parser and Command tightly coupled?

Internals/Implementation

Enhancements

1. Implement CommandToFunction helper method

2. Create Parameter#toOptionalParameter methods

aidoxe-123 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

JoeyChenSmart Sep 29, 2020 • edited

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

Jh123x left a comment • edited

Choose a reason for hiding this comment

Jh123x 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

Jh123x left a comment

Choose a reason for hiding this comment

Jh123x commented Oct 2, 2020

JoeyChenSmart commented Sep 27, 2020 •

edited

JoeyChenSmart Sep 29, 2020 •

edited

Jh123x left a comment •

edited