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.

Sign up for GitHub

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

Jump to bottom

Update DG model and delete feature #55

Merged
merged 11 commits into from
Oct 19, 2020
Merged

Update DG model and delete feature #55

merged 11 commits into from
Oct 19, 2020

Conversation

shaokiat
Copy link

  • Add Delete feature in developer guide
  • Update DeleteSequenceDiagram
  • Update ModelClassDiagram and BetterModelClassDiagram

@shaokiat shaokiat added documentation Improvements or additions to documentation type.Task priority.Medium labels Oct 15, 2020
@shaokiat shaokiat self-assigned this Oct 15, 2020
@shaokiat shaokiat linked an issue Oct 15, 2020 that may be closed by this pull request
Add Delete Feature in Developer Guide and Update Model Diagrams #54
Closed
mhdsyfq77
mhdsyfq77 requested changes Oct 17, 2020
View reviewed changes
Copy link

@mhdsyfq77 mhdsyfq77 left a comment

Choose a reason for hiding this comment

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

generally well done bro 👍🏽

just one thing i noticed, doesn't our delete command parse an index rather than the module code? or did we decide we are going to change that and i totally forgot LOL

yan-soon
yan-soon reviewed Oct 17, 2020
View reviewed changes
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/diagrams/DeleteSequenceDiagram.puml Outdated
Comment on lines 16 to 21
[-> LogicManager : execute("delete 1")
[-> LogicManager : execute("delete c/CS2103T")
activate LogicManager

LogicManager -> AddressBookParser : parseCommand("delete 1")
activate AddressBookParser
LogicManager -> GradPadParser : parseCommand("delete c/CS2103T")
activate GradPadParser
Copy link

Choose a reason for hiding this comment

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

I think our current delete system still works with indexes, but possibly can KIV for future updates?

Copy link
Author

Choose a reason for hiding this comment

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

Yup for now it still works with indexes but I think we are planning to change it to ModuleCode in the near future so I just change the documentation first.

@mhdsyfq77 mhdsyfq77 requested review from mhdsyfq77 and removed request for mhdsyfq77 October 17, 2020 08:34
@shaokiat
Merge branch 'master' into update-DG-model-delete
7f23a3e 
# Conflicts:
#	docs/diagrams/DeleteSequenceDiagram.puml
#	docs/images/DeleteSequenceDiagram.png
@codecov-io
Copy link

codecov-io commented Oct 17, 2020 •
edited

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #55   +/-   ##
=========================================
  Coverage     71.94%   71.94%           
  Complexity      377      377           
=========================================
  Files            69       69           
  Lines          1155     1155           
  Branches        111      111           
=========================================
  Hits            831      831           
  Misses          294      294           
  Partials         30       30

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 0f8c638...02c7f62. Read the comment docs.

Silvernitro
Silvernitro reviewed Oct 17, 2020
View reviewed changes
Copy link

@Silvernitro Silvernitro left a comment

Choose a reason for hiding this comment

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

Mostly LGTM!

Also I think there's a duplicate ModelClassDiagram.png in the root directory 🤔

docs/DeveloperGuide.md Outdated
1. Test case: `delete 1`<br>
Expected: First contact is deleted from the list. Details of the deleted contact shown in the status message. Timestamp in the status bar is updated.
1. Test case: `delete CS2103T`<br>
Expected: CS2103T module is deleted from 'Current Modules'. Details of the deleted module shown in the status message. Timestamp in the status bar is updated.
Copy link

Choose a reason for hiding this comment

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

What does the timestamp in the status bar refer to ah?

Copy link
Author

Choose a reason for hiding this comment

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

I think it has got to do with the JavaFX status bar but I don't see it in the GUI

Copy link
Author

Choose a reason for hiding this comment

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

Okay I'll remove it since its not needed.

@shaokiat shaokiat merged commit 3d94aac into AY2021S1-CS2103T-T09-1:master Oct 19, 2020
@shaokiat shaokiat deleted the update-DG-model-delete branch November 3, 2020 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yan-soon yan-soon yan-soon left review comments

@Silvernitro Silvernitro Silvernitro approved these changes

@mhdsyfq77 mhdsyfq77 mhdsyfq77 approved these changes

Assignees

@shaokiat shaokiat

Labels
documentation Improvements or additions to documentation hacktoberfest-accepted priority.Medium type.Task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Delete Feature in Developer Guide and Update Model Diagrams
5 participants
@shaokiat @codecov-io @Silvernitro @yan-soon @mhdsyfq77