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

Update UG #195

Merged
merged 7 commits into from
Nov 4, 2020
Merged

Update UG #195

merged 7 commits into from
Nov 4, 2020

Conversation

shadowezz
Copy link

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 3, 2020

Codecov Report

Merging #195 into master will increase coverage by 0.71%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #195      +/-   ##
============================================
+ Coverage     53.13%   53.85%   +0.71%     
- Complexity      744      751       +7     
============================================
  Files           166      166              
  Lines          3122     3123       +1     
  Branches        349      347       -2     
============================================
+ Hits           1659     1682      +23     
+ Misses         1353     1333      -20     
+ Partials        110      108       -2     
Impacted Files Coverage Δ Complexity Δ
...java/seedu/address/model/person/person/Person.java 97.50% <ø> (ø) 20.00 <0.00> (ø)
...ess/model/deliverable/deliverable/Deliverable.java 69.81% <50.00%> (-1.35%) 17.00 <0.00> (ø)
...a/seedu/address/model/meeting/meeting/Meeting.java 88.67% <100.00%> (+2.96%) 23.00 <0.00> (-1.00) ⬆️
...du/address/logic/commands/meeting/EditCommand.java 81.25% <0.00%> (-0.24%) 12.00% <0.00%> (ø%)
...ddress/logic/commands/deliverable/EditCommand.java 93.42% <0.00%> (-0.09%) 12.00% <0.00%> (ø%)
...edu/address/logic/commands/person/EditCommand.java 95.65% <0.00%> (-0.07%) 12.00% <0.00%> (ø%)
...seedu/address/model/person/ModelPersonManager.java 94.73% <0.00%> (+0.09%) 24.00% <0.00%> (ø%)
...edu/address/model/meeting/ModelMeetingManager.java 45.61% <0.00%> (+0.97%) 10.00% <0.00%> (ø%)
...ess/model/deliverable/ModelDeliverableManager.java 63.93% <0.00%> (+3.22%) 14.00% <0.00%> (+1.00%)
...ddress/model/deliverable/deliverable/Deadline.java 50.00% <0.00%> (+16.66%) 4.00% <0.00%> (+1.00%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0886dc4...9761b8e. Read the comment docs.

gabztcr
gabztcr previously requested changes Nov 3, 2020
Copy link

@gabztcr gabztcr left a comment

Choose a reason for hiding this comment

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

Added comments 😎


<div markdown="block" class="alert alert-info">

**:information_source: Note:** You cannot add a contact with the same name and email as another contact in your
Copy link

Choose a reason for hiding this comment

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

Suggest to edit to Chrystal's phrasing: You cannot add a contact with the same name and email as another existing contact. Here and elsewhere for the other commands/models.

Copy link

Choose a reason for hiding this comment

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

May want to leave out the example to keep the Note short. What do you think?

@@ -202,13 +216,27 @@ Format: `add t/TITLE m/MILESTONE by/DEADLINE [d/DESCRIPTION] [c/CONTACTS]`
* `MILESTONE` is the milestone tagged to the deliverable.
* `MILESTONE` takes in numerical values separated by periods, e.g. `1.3`, `14.2.1`.
* `DEADLINE` is the due date time of the deliverable in DD-MM-YYYY HH:mm format.
* `DEADLINE` can be in the past but must not be earlier than the year 2019.
Copy link

Choose a reason for hiding this comment

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

Did we edit the code to validate this when we parse the datetime?

Copy link
Author

Choose a reason for hiding this comment

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

yes I did in the other PR


* You cannot add a deliverable with the same title as another deliverable in your deliverable list.<br>
e.g. `add t/Build Login page m/1.0 by/10-11-2020 18:00` will not work if there is another deliverable with the title
`Build Login page` in your deliverable list.
Copy link

Choose a reason for hiding this comment

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

May want to consider leaving out examples as mentioned previously.

@shadowezz shadowezz linked an issue Nov 4, 2020 that may be closed by this pull request
@shadowezz shadowezz modified the milestones: mid-v1.4, v1.4 Nov 4, 2020
* `add r/stk n/Betsy Crowe e/betsybet872@pmail.com`
adds a stakeholder with the name `Betsy Crowe` and email `betsybet872@pmail.com`.

<div markdown="span" class="alert alert-primary">:bulb:
Copy link

Choose a reason for hiding this comment

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

I think tip should be right after Note, not Examples
https://ay2021s1-cs2103t-f11-2.github.io/tp/UserGuide.html#adding-a-meeting-add

Copy link

@chrystalquek chrystalquek left a comment

Choose a reason for hiding this comment

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

LGTM

@shadowezz shadowezz dismissed gabztcr’s stale review November 4, 2020 10:39

Changes have been made.

Productiv automation moved this from PRs Pending Approvals to PRs approved / Ready for Testing Nov 4, 2020
@shadowezz shadowezz merged commit e55c077 into AY2021S1-CS2103T-F11-2:master Nov 4, 2020
Productiv automation moved this from PRs approved / Ready for Testing to Issues Done / PRs Merged (After Testing) Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Productiv
Issues Done / PRs Merged (After Testing)
4 participants