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

[F10-4] MeNUS #73

Open
wants to merge 2,215 commits into
base: master
Choose a base branch
from
Open

Conversation

azhikai
Copy link

@azhikai azhikai commented Sep 22, 2018

@rebstan97 @m4dkip @yican95 @HyperionNKJ
Closed #3 due to invalidity.

Copy link

@dalessr dalessr left a comment

Choose a reason for hiding this comment

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

Good job 👍 For now all the changes are fine. Can start to update user guide (i.e. features for each version) and developer guide (i.e. user story, use cases, NFR...) accordingly.

@azhikai
Copy link
Author

azhikai commented Sep 23, 2018

@dalessr Thank you. Yes, we have started doing that but not quite complete yet. 👍

@damithc
Copy link

damithc commented Sep 24, 2018

Good job on following the guidelines on team member photos closely. Your team is one of the few to have done it so far. 👍

Copy link

@dalessr dalessr left a comment

Choose a reason for hiding this comment

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

Good job 👍 @azhikai @rebstan97 @m4dkip @yican95 @HyperionNKJ

Some comments:

  • User Guide: add description and examples for all the features due to consistency (i.e. help). Can omit examples of trivial features (i.e. list). Make the format of the tips consistent (can follow the format in 5.2). Some command words are quite long (i.e. list-ingredients-low), maybe can think of some shorter ones easier for users to remember? Also, remember to add "Coming in v2.0" for features not yet implemented.
  • Developer Guide: can reorder user stories so that all high level features are displayed on top, followed by medium level features and low level features. Some use cases are incorrect. Remember that the steps in MSS reflect the interaction between user and system, which indicates that 2 system actions cannot appear consecutively.
  • Also, remember to resolve conflicts in developer guide.

@azhikai
Copy link
Author

azhikai commented Oct 6, 2018

@dalessr Thank you on behalf of my team for the feedback. We will work on them for the upcoming milestone. 👍
For the conflict, I think it's a conflict between our team repo's version and the AB4's version though, because on our end there's no conflict at all.

@dalessr
Copy link

dalessr commented Oct 6, 2018

For the conflict, I think it's a conflict between our team repo's version and the AB4's version though, because on our end there's no conflict at all.

Yup. Ok the conflict here is actually quite trivial, so can just resolve it because I think probably that is the reason why Travis CI test is not triggered in this PR.

@azhikai
Copy link
Author

azhikai commented Oct 8, 2018

Hi @dalessr , could you advice us on how to go about resolving the conflicts? We don't have write access to this repo and can't figure a way out.

@dalessr
Copy link

dalessr commented Oct 8, 2018

Hmm can you see the web editor link above Conflicting files? Can figure out the conflict first and do a commit on your org's master branch.

@azhikai
Copy link
Author

azhikai commented Oct 8, 2018

image
This is what I'm seeing.

@dalessr
Copy link

dalessr commented Oct 8, 2018

Uhh ok I see what happens. Never mind can just leave it. I will go directly to your team's repo to check Travis Build.

@azhikai
Copy link
Author

azhikai commented Oct 8, 2018

Uhh ok I see what happens. Never mind can just leave it. I will go directly to your team's repo to check Travis Build.

Thanks. Please let us know if there's anything we need to do to resolve this conflict. 👍

@azhikai
Copy link
Author

azhikai commented Oct 12, 2018

Hi @dalessr , not sure why but Netlify failed to deploy here. Sometimes it happened in our team org repo, but redeploying it fixes the issue. Also, still not sure why Travis isn't triggered.

Do you think it's better if we open a new PR or something? Kindly advice, thanks! 😄

@dalessr
Copy link

dalessr commented Oct 14, 2018

@azhikai Sorry for the late reply. Netlify sometimes fails in other PRs also, seems that it is not stable.

Travis cannot be triggered until the conflict is resolved. Since you have write access to your team's repo, can you try to merge the module's repo master branch into your team's repo master branch to resolved it?

@azhikai
Copy link
Author

azhikai commented Oct 14, 2018

@azhikai Sorry for the late reply. Netlify sometimes fails in other PRs also, seems that it is not stable.

Travis cannot be triggered until the conflict is resolved. Since you have write access to your team's repo, can you try to merge the module's repo master branch into your team's repo master branch to resolved it?

Will do that after submitting v1.2. 👍

@azhikai
Copy link
Author

azhikai commented Oct 15, 2018

@dalessr Fixed the conflict.

Copy link

@dalessr dalessr left a comment

Choose a reason for hiding this comment

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

@azhikai @rebstan97 @m4dkip @yican95 @HyperionNKJ Good job for v1.2 👍 Some comments:

README:

  • Remove the second point as this is an actual product designed for restaurant owners instead of se learning purpose.

User Guide:

  • Can put Coming in v2.0 features at the bottom of the command list
  • Should add command format and example for 5.43 also, even though you will not be implementing it.

Developer Guide:

  • Update/add diagrams where applicable. i.e.:
    image2
    (In your project, Person should become food item right?)
  • Add labels for all the newly added diagrams (i.e. the sequence diagrams in implementation section)

@azhikai
Copy link
Author

azhikai commented Oct 19, 2018

@dalessr Thank you for the review on behalf of the team.

We have noted your feedback and will make the necessary changes accordingly.

For Person, we're intending to remove it in v1.3 as it's not used at all.

@azhikai azhikai mentioned this pull request Oct 19, 2018
3 tasks
Bellaaarh pushed a commit to Bellaaarh/addressbook-level4 that referenced this pull request Oct 29, 2018
…aadit_changes

Add tests for Deadline, InterestCommand and InterestCommandParser
@azhikai
Copy link
Author

azhikai commented Oct 30, 2018

@dalessr We have began refactoring, may I check with you if it's okay if we don't refactor the package name? i.e. let it remain as seedu.address. But everything else has been refactored accordingly (no more mentions of address book, etc).

@dalessr
Copy link

dalessr commented Oct 30, 2018

Sure. It should be fine as long as address book is not showing in the documentations (user guide, developer guide, readme, about us...) as well as the product.

@azhikai
Copy link
Author

azhikai commented Oct 30, 2018

Sure. It should be fine as long as address book is not showing in the documentations (user guide, developer guide, readme, about us...) as well as the product.

Yup, everything related to "address book" has been removed from the code, UGDG, README, basically everywhere we can think of.

Thanks for the clarification! 👍

@azhikai
Copy link
Author

azhikai commented Oct 30, 2018

@dalessr Hi Shengran, for the releases, is it okay if we leave the "description" as blank? As seen https://github.com/CS2103-AY1819S1-F10-4/main/releases

Ideally in real scenario we don't, but was wondering if the description in releases play a part when comes to grading.

@azhikai
Copy link
Author

azhikai commented Nov 12, 2018

@dalessr On behalf the team, we'd like to extend our heartfelt gratitude to you for the help you've rendered us throughout the semester. I believe the team did our best, and hope that we were able to live up to your expectations.

Signing out for the last time. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants