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

Add Alfred Parsers #98

Merged
merged 31 commits into from Oct 17, 2019
Merged

Add Alfred Parsers #98

merged 31 commits into from Oct 17, 2019

Conversation

Abhiman2211
Copy link

Description

Implements all the parsers required by Alfred.

Checks

  • [] No errors when running the tests
  • [] Build and checkstyle passes
  • [] Run the app and ran 2 commands without failing
  • [] Written the baseline test cases for the PR

Changelog

  • []
  • []
  • []

Comments for Reviewer (if any)

@hcwong The ID-ing of each entity still doesn't work properly. Could you just check and see whether I did something wrong when generating the ID (maybe I am not using the latest way of generating the ID in case you changed it)
@brianyenna and @hcwong I can't create a team at all. It instantly says that the team already exists but nothing is written to Storage.

@Abhiman2211 Abhiman2211 added this to the v1.2 milestone Oct 16, 2019
@hcwong
Copy link

hcwong commented Oct 16, 2019

@brianyenna and @hcwong I can't create a team at all. It instantly says that the team already exists but nothing is written to Storage.

Could you give more details on how to reproduce that second error (what you keyed on etc)? 1st one is expected behaviour for now because the ID checks are in yet. It seems like a storage/Fx bug for now.

Copy link

@hcwong hcwong left a comment

Choose a reason for hiding this comment

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

Alright in general, but two things to note before merging apart from fixing the build.

  1. generate ID should now be called as a static method
  2. Standardization of command words. add Participant is addParticipant but in other places it does not seem to be the of the same format
    As for the bug with team, I think it is good to merge cos Participant is working, we'll fix it later

Also help me review #86 thanks. If it's okay just help me approve and merge thanks i dont have wifi

@Abhiman2211
Copy link
Author

Abhiman2211 commented Oct 17, 2019

Alright in general, but two things to note before merging apart from fixing the build.

  1. generate ID should now be called as a static method
  2. Standardization of command words. add Participant is addParticipant but in other places it does not seem to be the of the same format
    As for the bug with team, I think it is good to merge cos Participant is working, we'll fix it later

Also help me review #86 thanks. If it's okay just help me approve and merge thanks i dont have wifi

For point 2, all other commands except addParticipant actually use the entity ID straight away since once it's created we can just identify the participant with the ID unlike when we are still creating it. But you are right, there should be maximum standardisation. If you have any suggestion on how to change it I am completely open. I could change everything to the format "commandEntity" (like viewParticipant) but what I thought was that it just requires extra cases in the AlfredParser. If you think that doesn't matter tho, I can change it to such.

@Abhiman2211
Copy link
Author

@brianyenna and @hcwong I can't create a team at all. It instantly says that the team already exists but nothing is written to Storage.

Could you give more details on how to reproduce that second error (what you keyed on etc)? 1st one is expected behaviour for now because the ID checks are in yet. It seems like a storage/Fx bug for now.

Just a normal addTeam command "add team n/Team 01 s/Social pn/Emotion Train pt/Public Welfare l/22". The message displayed says "This team already exists in this Hackathon". I am still looking into it to see where the problem is coming from. But any help would be appreciated

@@ -27,7 +27,7 @@
+ CliSyntax.PREFIX_NAME + "John Doe "
+ CliSyntax.PREFIX_PHONE + "98765432 "
+ CliSyntax.PREFIX_EMAIL + "johnd@example.com "
+ CliSyntax.PREFIX_ORGANISATION + "Fill it up for me idk what this is...";
+ CliSyntax.PREFIX_ORGANISATION + "Google";

Choose a reason for hiding this comment

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

I added the Message Usage for the commands 😃

@@ -15,8 +15,8 @@
*/
public class DeleteMentorCommand extends DeleteCommand {

public static final String COMMAND_WORD = "delete";
public static final String MESSAGE_INVALID_MENTOR_DISPLAYED_INDEX = "The mentor ID provided is invalid";
public static final String MESSAGE_INVALID_MENTOR_DISPLAYED_INDEX = "The mentor ID provided is "

Choose a reason for hiding this comment

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

Oh so is it changed to just delete(the parser will automatically parse out the Entity type)? Do correct me if I am wrong hahaha 😄

Copy link
Author

Choose a reason for hiding this comment

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

yup, the command_word is just delete. So when you do delete P-1, it automatically knows that P is associated with participant so it deletes Participant with ID P-1

@@ -13,5 +13,6 @@
public static final String MESSAGE_PERSONS_LISTED_OVERVIEW = "%1$d persons listed!";
// to set restrictions as some operations can only be applied to some entity
public static final String MESSAGE_INVALID_TYPE = "The type of entity is invalid";
public static final String MESSAGE_INVALID_INDEX = "Index is not a non-zero unsigned integer.";

Choose a reason for hiding this comment

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

The double negative seems a little confusing, how about MESSAGE_INVALID_INDEX = "Invalid index! Index should be a non-zero unsigned integer." haha just throwing out ideas :)

Copy link

@justarock111 justarock111 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@brianyenna brianyenna left a comment

Choose a reason for hiding this comment

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

LGTM! With regard to the addTeam command problem, let's discuss it a little more during the meeting tomorrow. Easier tofigure out the problem that way :)

@Abhiman2211 Abhiman2211 merged commit 5bff88a into master Oct 17, 2019
@Abhiman2211 Abhiman2211 self-assigned this Oct 31, 2019
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.

None yet

4 participants