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

[CS2103-F11-4] NUScheduler #68

Open
wants to merge 216 commits into
base: master
Choose a base branch
from

Conversation

putaojuice
Copy link

@putaojuice putaojuice commented Feb 17, 2022

NUScheduler is a desktop app for Year 1 Computing students to assist with more efficient management of tasks and contacts, optimised for use via a Command Line Interface (CLI) while still having the benefits of a Graphical User Interface (GUI). If you can type fast, NUScheduler can schedule your tasks faster than traditional GUI apps.

@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

Merging #68 (51b8ad9) into master (4f7660a) will increase coverage by 2.85%.
The diff coverage is 81.20%.

@@             Coverage Diff              @@
##             master      #68      +/-   ##
============================================
+ Coverage     72.15%   75.01%   +2.85%     
- Complexity      399      540     +141     
============================================
  Files            70       87      +17     
  Lines          1232     1737     +505     
  Branches        125      186      +61     
============================================
+ Hits            889     1303     +414     
- Misses          311      377      +66     
- Partials         32       57      +25     
Impacted Files Coverage Δ
src/main/java/seedu/address/MainApp.java 0.00% <0.00%> (ø)
...n/java/seedu/address/commons/core/GuiSettings.java 69.23% <ø> (ø)
.../seedu/address/logic/parser/AddressBookParser.java 72.72% <0.00%> (-27.28%) ⬇️
src/main/java/seedu/address/model/Model.java 100.00% <ø> (ø)
.../java/seedu/address/model/util/SampleDataUtil.java 15.00% <0.00%> (-5.00%) ⬇️
src/main/java/seedu/address/ui/HelpWindow.java 0.00% <ø> (ø)
src/main/java/seedu/address/ui/MainWindow.java 0.00% <0.00%> (ø)
src/main/java/seedu/address/ui/TaskCard.java 0.00% <0.00%> (ø)
src/main/java/seedu/address/ui/TaskListPanel.java 0.00% <0.00%> (ø)
src/main/java/seedu/address/ui/UiManager.java 0.00% <0.00%> (ø)
... and 24 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 4f7660a...51b8ad9. Read the comment docs.

aditi2313 added a commit to aditi2313/tp that referenced this pull request Feb 24, 2022
The person equality check returns true if the name or the email
matches. The OR condition complicates the equality check done when
editing a person as it assumes isSamePerson() to have a transitive
relation.

Alternative 1: Improve the way the equality check is done in the edit
command to prevent the above bug. (PRs nus-cs2103-AY2122S2#68, nus-cs2103-AY2122S2#69)

Alternative 2: Simplify the equality check by removing the OR condition.
This is simpler but possibly too strict for the address book domain
i.e., increases the risk of duplicate entries for the same person
creeping into the address book

We decided to go with the alternative 2 because,
* It is important to keep the code base simple, to reduce the initial
learning curve of the students at the start of their project
* Students can introduce a more sophisticated equality check that
matches their product domain later.

Also, let's change the default name in PersonBuilder utility class.
This is because AddCommandIntegrationTest populates its model using
getTypicalPersons() which contains a person with the default name.
This test then creates another person using the default name, to be
added into the model, which is expected to pass. However, the above
update in equality causes this test to fail. Thus we need to change the
default name to a unique name that is not inside getTypicalPersons().
Also, the default email is changed for consistency in naming the
default values, it does not affect current test cases.
aditi2313 added a commit to aditi2313/tp that referenced this pull request Feb 25, 2022
The person equality check returns true if the name or the email
matches. The OR condition complicates the equality check done when
editing a person as it assumes isSamePerson() to have a transitive
relation.

Alternative 1: Improve the way the equality check is done in the edit
command to prevent the above bug. (PRs nus-cs2103-AY2122S2#68, nus-cs2103-AY2122S2#69)

Alternative 2: Simplify the equality check by removing the OR condition.
This is simpler but possibly too strict for the address book domain
i.e., increases the risk of duplicate entries for the same person
creeping into the address book

We decided to go with the alternative 2 because,
* It is important to keep the code base simple, to reduce the initial
learning curve of the students at the start of their project
* Students can introduce a more sophisticated equality check that
matches their product domain later.

Also, let's change the default name in PersonBuilder utility class.
This is because AddCommandIntegrationTest populates its model using
getTypicalPersons() which contains a person with the default name.
This test then creates another person using the default name, to be
added into the model, which is expected to pass. However, the above
update in equality causes this test to fail. Thus we need to change the
default name to a unique name that is not inside getTypicalPersons().
Also, the default email is changed for consistency in naming the
default values, it does not affect current test cases.
putaojuice and others added 20 commits February 27, 2022 15:48
- Update UserGuide with draft UG for v1.1
Update UserGuide and Skeletal PPP
Add acknowledgement and update _config.yml to fit project.
- Add AddTaskCommand

- Add AddTaskCommandParser

- Add model.task.Task

- Add model.TaskList

- Modify Model (Interface)

- Modify ModelManager
    - include TaskList in constructor
- Add javadoc comments

- Rearrange import statements
- Add AddTaskCommandTest
putaojuice and others added 30 commits April 11, 2022 18:02
- Minor fix to the UG command summary
- Add page breaks
- Update DG non-functional requirement based on the requirement stated on CS2103 website
Update PPP and Update Model Class Diagram to reflect Task
Add page break
Add page break
Remove Page break
Update Statistics
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.

5 participants