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

Fix AddParser #204

Merged
merged 12 commits into from
Nov 6, 2016
Merged

Fix AddParser #204

merged 12 commits into from
Nov 6, 2016

Conversation

burnflare
Copy link

Fixes #202

return new IncorrectCommand(String.format(MESSAGE_INVALID_COMMAND_FORMAT,
AddCommand.MESSAGE_USAGE));
} else {
return new AddCommand(args);
Copy link
Author

@burnflare burnflare Nov 5, 2016

Choose a reason for hiding this comment

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

@rachx, a quick fix I made up. Is this valid? Basically, if we don't fit the 2 criteria, just send the entire incoming string directly to AddCommand(Str)

@burnflare
Copy link
Author

@rachx Updated with quotation support. Can you check?

@rachx
Copy link

rachx commented Nov 5, 2016

Schedule should be fine right?

How about the following input

add return "animal farm". it will only save "animal farm"
@fanwgwg i think we might need to change our user guide examples and screenshots after our decision

I am thinking if we should save with the inverted commas
e.g. task name "copy assignment from cs2101 to cs2103". it makes the first and last word un-searchable.

Instead of using " ", can we use something that the user is unlikely to use and easy to type
e.g. add --drop by seven eleven -- (2 or 3 leading and trailing slashes but don't save to title). thoughts?

@rachx
Copy link

rachx commented Nov 5, 2016

oh ya, you might need to change the add tests in logic manager test

@burnflare
Copy link
Author

Schedule is fine.

Possible... we could change the delimiter to something else. @INCENDE?

@fanwgwg
Copy link

fanwgwg commented Nov 5, 2016

@rachx for safe, I'll just remove the quotation marks in the screenshots

@burnflare
Copy link
Author

@fanwgwg Don't remove them yet. Let's come to a decision first so that you don't need to do double work

@fanwgwg
Copy link

fanwgwg commented Nov 5, 2016

okay that's fine!

@burnflare
Copy link
Author

@rachx please verify

rachx added 2 commits November 6, 2016 13:36
…and to format, update logic manager test to test escape character
if (!dateTimeMap.containsKey(ARGS_FROM)
&& !dateTimeMap.containsKey(ARGS_TO)
&& !dateTimeMap.containsKey(ARGS_BY)) {
if (!hasDeadlineKeyword && !hasStartTimeKeyword && !hasEndTimeKeyword) {
Copy link
Author

Choose a reason for hiding this comment

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

Nice!

@rachx
Copy link

rachx commented Nov 6, 2016

@burnflare @fanwgwg @INCENDE
Everyone please verify.

I change the escape character from " " to ' ' only (easier to type and people can still use double inverted commas as normal, no need to redo screenshot)

so

  1. add 'copy notes from 2101 to 2103' will add the task copy notes from 2101 to 2103
  2. natty will still parse add copy notes from 2101 to 2103 => task copy notes with time
  3. if there is no ' ' , it will expect
    a) no by, from and to keywords with time behind
    b) by keyword with time behind only
    c) from and to keyword with time behind only
    (this is for more consistent behavior)

Some small problems

  1. add by 8pm. It will add the task name by 8pm instead of complaining incorrect format is that fine?
  2. add return 'book' will be saved as book instead of return 'book'. just get people to use double inverted commas instead?

@burnflare burnflare self-assigned this Nov 6, 2016
@rachx
Copy link

rachx commented Nov 6, 2016

hmmm fix and decide on this and check that everything works after fixing gcal?

@burnflare
Copy link
Author

I did some light testing and it looks good. @INCENDE Could you test this too?

@rachx
Copy link

rachx commented Nov 6, 2016

how about add by 8pm?

@burnflare
Copy link
Author

Thinking about it.... there's noting wrong with accepting add by 8pm 💯

If user inputs add by 8pm, we expect there to be a title before by 8pm. But since we didn't see a title, we ignore the date time and insert the entire thing as a title instead. IMO, this behaviour is better than the opposite (create a blank event that's due at 8pm).

@rachx
Copy link

rachx commented Nov 6, 2016

Okay. Initially, I was thinking of incorrect format since the name is missing. It recognises add from 4pm to 8pm as incorrect format
Lets wait for @INCENDE to merge

@burnflare burnflare removed their assignment Nov 6, 2016
@burnflare
Copy link
Author

Assigning to @rachx as I can't approve my own PR. Feel free to approve and merge when you're ready.

@INCENDE
Copy link

INCENDE commented Nov 6, 2016

good to go

@burnflare burnflare merged commit 529a4b8 into master Nov 6, 2016
@burnflare burnflare deleted the parsing-issues branch November 6, 2016 16:31
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.

4 participants