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

Fixes #47 #53

Merged
merged 12 commits into from Oct 7, 2020
Merged

Fixes #47 #53

merged 12 commits into from Oct 7, 2020

Conversation

VaishakAnand
Copy link

@VaishakAnand VaishakAnand commented Oct 7, 2020

Updated AdditionalDetail, ClassVenue and ClassTime, and AdditionalDetail tests (that were initially from Tag).
Replaced all Tag occurences with AdditionalDetail

@VaishakAnand VaishakAnand added this to the v1.2 milestone Oct 7, 2020
Removed a few unnoticed Tag mentions
@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@e6533c6). Click here to learn what that means.
The diff coverage is 5.17%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #53   +/-   ##
=========================================
  Coverage          ?   66.42%           
  Complexity        ?      370           
=========================================
  Files             ?       73           
  Lines             ?     1230           
  Branches          ?      120           
=========================================
  Hits              ?      817           
  Misses            ?      373           
  Partials          ?       40           
Impacted Files Coverage Δ Complexity Δ
...java/seedu/address/logic/commands/EditCommand.java 96.72% <ø> (ø) 12.00 <0.00> (?)
.../seedu/address/logic/parser/EditCommandParser.java 76.00% <0.00%> (ø) 7.00 <0.00> (?)
...in/java/seedu/address/logic/parser/ParserUtil.java 66.66% <0.00%> (ø) 10.00 <0.00> (?)
...a/seedu/address/model/student/admin/ClassTime.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../seedu/address/model/student/admin/ClassVenue.java 30.00% <0.00%> (ø) 2.00 <0.00> (?)
.../java/seedu/address/model/util/SampleDataUtil.java 0.00% <ø> (ø) 0.00 <0.00> (?)
...u/address/storage/JsonAdaptedAdditionalDetail.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...java/seedu/address/storage/JsonAdaptedStudent.java 91.42% <ø> (ø) 10.00 <0.00> (?)
.../address/model/student/admin/AdditionalDetail.java 30.00% <30.00%> (ø) 2.00 <2.00> (?)

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 e6533c6...0854ad2. Read the comment docs.

Copy link

@hogantan hogantan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@AlexCQY AlexCQY left a comment

Choose a reason for hiding this comment

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

LGTM

Fixed wrong syntax for Year
Changed more tag references to AdditionalDetail refs
Copy link

@ong-yinggao98 ong-yinggao98 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I guess we will revert it back to allow alphanumerics bah

@@ -9,14 +9,22 @@
import seedu.address.model.student.Year;
import seedu.address.model.student.admin.Admin;


/**
* Contains utility methods for populating {@code AddressBook} with sample data.
Copy link

Choose a reason for hiding this comment

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

Should we remove the traces of address book?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I'll remove it in my next PR.

@@ -22,6 +22,7 @@
import seedu.address.model.student.Year;
import seedu.address.model.student.admin.Admin;


/**
* Edits the details of an existing person in the address book.
Copy link

Choose a reason for hiding this comment

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

here also but i think rmb to change later

Copy link

@csiongn csiongn left a comment

Choose a reason for hiding this comment

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

LGTM

@csiongn csiongn merged commit 1e1687b into AY2021S1-CS2103T-W15-2:master Oct 7, 2020
@ong-yinggao98 ong-yinggao98 linked an issue Oct 7, 2020 that may be closed by this pull request
@VaishakAnand VaishakAnand deleted the v1.2 branch November 8, 2020 09:24
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.

Field redundancy in Student class
6 participants