Command parsers: use exceptions for error handling #305 #333

yamgent · 2017-03-11T03:05:06Z

Fixes #305.

Proposed merge commit message:

All CommandParser#parse(String) return a generic Command type, instead
of the specific command that they are tasked to parse (for example,
AddCommand for AddCommandParser).

This is because the methods might also return an IncorrectCommand if it
encounters a parse error. This causes an unnecessary loss of specificity
in the return value type if the parsing is successful (e.g. receiving a
Command object instead of an AddCommand object), and in the error info
if the parsing failed (e.g. receiving an IncorrectCommand instead of a
specific Exception object containing the exact stack trace).

Let's change all CommandParser#parse(String) methods to return their
specific command type, and throw a ParseErrorException instead when
encountering an error, to allow the caller to deal with it.

mention-bot · 2017-03-11T03:05:07Z

@yamgent, thanks for your PR! By analyzing the history of the files in this pull request, we identified @damithc, @yl-coder and @MightyCupcakes to be potential reviewers.

CanIHasReview-bot · 2017-03-11T03:18:25Z

v1

@yamgent submitted v1 for review.

[v1 1/3] Add ParseErrorException
[v1 2/3] Modify all CommandParser#parse(String) to throw ParseErrorException
[v1 3/3] Change CommandParser#parse(String) return type to be more specific

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/333/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

PierceAndy

[v1 1/3]
Commit message:

CommandParser -> command parser (referring to command parser classes in general)
return an IncorrectCommand -> return an IncorrectCommand object
This behavior is not explicit... -> This error handling behavior is not explicit. Instead of throwing an exception as expected, a child class object (IncorrectCommand) of the return type (Command) is returned.

PierceAndy · 2017-03-11T14:45:49Z

I'm not sure referring to command parsers in general as CommandParser is a good practice, as that might imply that there is a CommandParser parent class.

MightyCupcakes

No further comments from me

pyokagan · 2017-03-13T03:15:35Z

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

@@ -17,11 +15,11 @@
     * Parses the given {@code String} of arguments in the context of the DeleteCommand
     * and returns an DeleteCommand object for execution.
     */
-    public Command parse(String args) {
+    public DeleteCommand parse(String args) throws ParseErrorException  {


Two many spaces.

pyokagan · 2017-03-13T03:16:08Z

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

+/**
+ * Represents a parse error encountered by a parser.
+ */
+public class ParseErrorException extends Exception {


Why not just call it ParseException? Exceptions are supposed to only be used for errors anyway, so the Error is implied.

pyokagan · 2017-03-13T03:17:21Z

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

+                return new IncorrectCommand(MESSAGE_UNKNOWN_COMMAND);
+            }
+        } catch (ParseErrorException parserException) {
+            return new IncorrectCommand(parserException.getMessage());


Rather than catching it here, propagate it out instead? No reason why we have to go to the trouble of wrapping the exception into a command. We also lose some information as well (e.g. the stacktrace)

Reading further through the diff, however, I see that this commit still punts out at returning an IncorrectCommand, which is strange since the commit message promised me that all command parsers would now throw a ParseException? Unless Parser is not considered a command parser?

CanIHasReview-bot · 2017-03-14T04:33:44Z

v2

@yamgent submitted v2 for review.

[v2 1/4] Add ParseException
[v2 2/4] Modify all command parsers' parse(String) to throw ParseException
[v2 3/4] Change command parsers' parse(String) return type to be more specific
[v2 4/4] Modify Parser#parseCommand(String) to not wrap ParseException

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/333/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

PierceAndy

[v2 1/4]
Commit message:

instead of returning an IncorrectCommand -> instead of returning an IncorrectCommand object
found a parse error -> encountered a parse error?
more explicit stated -> more explicitly stated or more explicit

PierceAndy

[v2 4/4]
Commit message:

into an IncorrectCommand -> into an IncorrectCommand object

MightyCupcakes · 2017-03-14T05:34:21Z

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

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


create a new sub package seedu.address.logic.parser.exceptions and put this exception in the package to be consistent with the others?

CanIHasReview-bot · 2017-03-14T06:52:50Z

v3

@yamgent submitted v3 for review.

[v3 1/4] Add ParseException
[v3 2/4] Modify all command parsers' parse(String) to throw ParseException
[v3 3/4] Change command parsers' parse(String) return type to be more specific
[v3 4/4] Modify Parser#parseCommand(String) to not wrap ParseException

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/333/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

MightyCupcakes

[v3 1/4] Commit message:

Let's add a ParseException, allowing command parsers to throw this
exception, instead of returning an IncorrectCommand object, to tell
the caller that it has encountered a parse error. Therefore, the error
handling behavior of all command parsers' parse(String) methods can be
more explicit.

The opening sentence is too long.

Let's add the ParseException class which will allow the various command parsers to throw this exception instead of returning an IncorrectCommand object. This will inform the caller that a parse error has occurred.

CanIHasReview-bot · 2017-05-23T01:49:09Z

v15

@yamgent submitted v15 for review.

[v15 1/3] Modify all command parsers' parse(String) to throw ParseException
[v15 2/3] Change command parsers' parse(String) return type to be more specific
[v15 3/3] Remove IncorrectCommand

(📚 Archive) (📈 Interdiff between v14 and v15)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/333/15/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

yamgent · 2017-05-23T01:51:27Z

@damithc I removed IncorrectCommand for this iteration, need you to take a look at it again to approve the new design decision.

CanIHasReview-bot · 2017-05-25T03:07:59Z

v16

@yamgent submitted v16 for review.

[v16 1/3] Modify all command parsers' parse(String) to throw ParseException
[v16 2/3] Change command parsers' parse(String) return type to be more specific
[v16 3/3] Remove IncorrectCommand

(📚 Archive) (📈 Interdiff between v15 and v16)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/333/16/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

yamgent · 2017-05-25T03:19:09Z

v16: Rebased

@damithc Just a reminder for the review. :)

damithc

I'm ok with the code. @throws ParseException if any illegal values are found sounds a bit odd though (feels like "shouldn't we throw IllegalValueException in that case?") May be ... if the command does not conform the the exapected format or something like that? I'll leave up to you

All command parsers return an IncorrectCommand object when they encounter a parse error. The parse error can be made more visible if an exception was thrown instead. Otherwise, the caller is unable to handle any parsing errors such as IllegalValueException. Context information, such as the stacktrace, is also lost when wrapping the exception instead of returning it directly. Let's change all command parsers' parse(String) methods such that they throw ParseException instead of returning an IncorrectCommand object.

All command parsers' parse(String) methods return the base Command type, instead of the specific command that they are tasked to parse (for example, AddCommand for AddCommandParser). Let's change all command parsers' parse(String)'s return type from the generic Command type to their specific command type that they are tasked to parse.

Parser throws a ParseException instead of IncorrectCommand when encountering an unrecognized command. IncorrectCommand is no longer in use anywhere. Let's remove IncorrectCommand.

CanIHasReview-bot · 2017-05-29T00:44:58Z

v17

@yamgent submitted v17 for review.

[v17 1/3] Modify all command parsers' parse(String) to throw ParseException
[v17 2/3] Change command parsers' parse(String) return type to be more specific
[v17 3/3] Remove IncorrectCommand

(📚 Archive) (📈 Interdiff between v16 and v17)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/333/17/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent force-pushed the 305-parsers-exceptions branch from abf8142 to 351cd40 Compare March 11, 2017 03:08

CanIHasReview-bot added the s.ToReview label Mar 11, 2017

yamgent requested review from PierceAndy and MightyCupcakes March 11, 2017 03:18

PierceAndy suggested changes Mar 11, 2017

View reviewed changes

MightyCupcakes reviewed Mar 12, 2017

View reviewed changes

MightyCupcakes added s.Ongoing and removed s.ToReview labels Mar 12, 2017

pyokagan reviewed Mar 13, 2017

View reviewed changes

yamgent force-pushed the 305-parsers-exceptions branch 2 times, most recently from f698917 to 9b44ce9 Compare March 14, 2017 04:25

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 14, 2017

CanIHasReview-bot requested a review from PierceAndy March 14, 2017 04:33

PierceAndy suggested changes Mar 14, 2017

View reviewed changes

MightyCupcakes suggested changes Mar 14, 2017

View reviewed changes

MightyCupcakes added s.Ongoing and removed s.ToReview labels Mar 14, 2017

yamgent force-pushed the 305-parsers-exceptions branch from 9b44ce9 to 9fc4133 Compare March 14, 2017 06:45

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 14, 2017

CanIHasReview-bot requested review from PierceAndy and MightyCupcakes March 14, 2017 06:52

MightyCupcakes reviewed Mar 14, 2017

View reviewed changes

yamgent removed the s.ToReview label May 23, 2017

yamgent force-pushed the 305-parsers-exceptions branch from a048bf4 to 8124f0b Compare May 23, 2017 01:37

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 23, 2017

CanIHasReview-bot requested a review from Zhiyuan-Amos May 23, 2017 01:49

Zhiyuan-Amos approved these changes May 23, 2017

View reviewed changes

yamgent requested a review from damithc May 23, 2017 01:51

yamgent force-pushed the 305-parsers-exceptions branch 2 times, most recently from caaab15 to 76c4505 Compare May 25, 2017 02:42

damithc approved these changes May 25, 2017

View reviewed changes

yamgent added s.ToMerge and removed s.ToReview labels May 26, 2017

yamgent force-pushed the 305-parsers-exceptions branch from 76c4505 to e014124 Compare May 26, 2017 08:32

yamgent added 3 commits May 26, 2017 16:35

Remove IncorrectCommand

3b626bb

Parser throws a ParseException instead of IncorrectCommand when encountering an unrecognized command. IncorrectCommand is no longer in use anywhere. Let's remove IncorrectCommand.

yamgent force-pushed the 305-parsers-exceptions branch from e014124 to 3b626bb Compare May 26, 2017 08:35

CanIHasReview-bot added s.ToReview and removed s.ToMerge labels May 29, 2017

CanIHasReview-bot requested a review from pyokagan May 29, 2017 00:45

yamgent removed the request for review from pyokagan May 29, 2017 00:45

yamgent added s.ToMerge and removed s.ToReview labels May 29, 2017

yamgent merged commit cfc8ecc into se-edu:master May 29, 2017

yamgent deleted the 305-parsers-exceptions branch May 29, 2017 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command parsers: use exceptions for error handling #305 #333

Command parsers: use exceptions for error handling #305 #333

yamgent commented Mar 11, 2017 •

edited

Loading

mention-bot commented Mar 11, 2017

CanIHasReview-bot commented Mar 11, 2017

PierceAndy left a comment •

edited

Loading

PierceAndy commented Mar 11, 2017

MightyCupcakes left a comment

pyokagan Mar 13, 2017

pyokagan Mar 13, 2017

pyokagan Mar 13, 2017

pyokagan May 5, 2017

CanIHasReview-bot commented Mar 14, 2017

PierceAndy left a comment •

edited

Loading

PierceAndy left a comment

MightyCupcakes Mar 14, 2017

CanIHasReview-bot commented Mar 14, 2017

MightyCupcakes left a comment

CanIHasReview-bot commented May 23, 2017

yamgent commented May 23, 2017

CanIHasReview-bot commented May 25, 2017

yamgent commented May 25, 2017

damithc left a comment

CanIHasReview-bot commented May 29, 2017

Command parsers: use exceptions for error handling #305 #333

Command parsers: use exceptions for error handling #305 #333

Conversation

yamgent commented Mar 11, 2017 • edited Loading

mention-bot commented Mar 11, 2017

CanIHasReview-bot commented Mar 11, 2017

v1

PierceAndy left a comment • edited Loading

Choose a reason for hiding this comment

PierceAndy commented Mar 11, 2017

MightyCupcakes left a comment

Choose a reason for hiding this comment

pyokagan Mar 13, 2017

Choose a reason for hiding this comment

pyokagan Mar 13, 2017

Choose a reason for hiding this comment

pyokagan Mar 13, 2017

Choose a reason for hiding this comment

pyokagan May 5, 2017

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 14, 2017

v2

PierceAndy left a comment • edited Loading

Choose a reason for hiding this comment

PierceAndy left a comment

Choose a reason for hiding this comment

MightyCupcakes Mar 14, 2017

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 14, 2017

v3

MightyCupcakes left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented May 23, 2017

v15

yamgent commented May 23, 2017

CanIHasReview-bot commented May 25, 2017

v16

yamgent commented May 25, 2017

damithc left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented May 29, 2017

v17

yamgent commented Mar 11, 2017 •

edited

Loading

PierceAndy left a comment •

edited

Loading

PierceAndy left a comment •

edited

Loading