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 DG for CountryNotesManager #245

Merged
merged 14 commits into from
Oct 28, 2020

Conversation

raysonkoh
Copy link

@raysonkoh raysonkoh commented Oct 26, 2020

Description

Include Class Diagram for CountryNote-related classes in model + Sequence Diagram for Country Note Add.

Fixes #238

Testing

No tests were added.

Remarks

NIL

@raysonkoh raysonkoh added the type.documentation Improvements or additions to documentation label Oct 26, 2020
@raysonkoh raysonkoh added this to the v1.3 milestone Oct 26, 2020
@raysonkoh raysonkoh self-assigned this Oct 26, 2020
@raysonkoh raysonkoh added this to In progress in v1.3 via automation Oct 26, 2020
@codecov
Copy link

codecov bot commented Oct 26, 2020

Codecov Report

Merging #245 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #245   +/-   ##
=========================================
  Coverage     87.47%   87.47%           
  Complexity      900      900           
=========================================
  Files           109      109           
  Lines          2299     2299           
  Branches        251      251           
=========================================
  Hits           2011     2011           
  Misses          218      218           
  Partials         70       70           

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 ed896fc...c0586c8. Read the comment docs.

@raysonkoh raysonkoh marked this pull request as ready for review October 27, 2020 01:23
Copy link

@tankangliang tankangliang left a comment

Choose a reason for hiding this comment

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

Regarding the sequence diagram, I'm guessing the intermediate portions where AddressBookParser calls CountryNoteAddCommandParser is supposed to be abstracted out for brevity? Also, there is an intermediate call to AddressBook from Model before CountryNotesManager.


Given below is a sequence diagram that shows how the `country note add` command works.
For brevity, the full command `country note add c/COUNTRY_CODE nt/NOTE_STRING` will be substituted by `country note add`.
Note that the `AddressBookParser` parses the user input and returns a `CountryNote` object, which will be passed as an argument to the constructor of `CountryNoteAddCommand`.

Choose a reason for hiding this comment

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

Slightly inaccurate as AddressBookParser returns a Command type. Maybe talk about CountryNoteAddCommandParser which extracts Country and Note into CountryNote?

Copy link
Author

Choose a reason for hiding this comment

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

Yea thanks for catching that mistake, will mention about CountryNoteAddCommandParser and also say that it will be excluded from the sequence diagram for brevity.

Added the intermediate call to Addressbook#addCountryNote(countryNote)

v1.3 automation moved this from In progress to Review in progress Oct 27, 2020
Copy link

@tankangliang tankangliang left a comment

Choose a reason for hiding this comment

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

Looks good! Not sure if the sizing of the diagram will be an issue but can be merged for now.

v1.3 automation moved this from Review in progress to Reviewer approved Oct 28, 2020
@raysonkoh raysonkoh merged commit 4d61dad into AY2021S1-CS2103T-F11-4:master Oct 28, 2020
v1.3 automation moved this from Reviewer approved to Done Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.documentation Improvements or additions to documentation
Projects
No open projects
v1.3
Done
Development

Successfully merging this pull request may close these issues.

Update DG for country-related components
2 participants