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

Fix the esort and emark bug #231

Merged
merged 5 commits into from
Nov 7, 2021
Merged

Fix the esort and emark bug #231

merged 5 commits into from
Nov 7, 2021

Conversation

chunweii
Copy link
Collaborator

@chunweii chunweii commented Nov 7, 2021

Closes #222
Closes #234

@chunweii chunweii added type.Bug Something isn't working severity.High A flaw that affects most users and causes major problems for users labels Nov 7, 2021
@chunweii chunweii added this to the v1.4 milestone Nov 7, 2021
@chunweii chunweii added this to In progress in tP Project Dashboard via automation Nov 7, 2021
@codecov
Copy link

codecov bot commented Nov 7, 2021

Codecov Report

Merging #231 (d1f6bb4) into master (2ff0fe7) will decrease coverage by 0.08%.
The diff coverage is 98.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #231      +/-   ##
============================================
- Coverage     75.76%   75.67%   -0.09%     
+ Complexity     1166     1157       -9     
============================================
  Files           129      129              
  Lines          3420     3421       +1     
  Branches        450      451       +1     
============================================
- Hits           2591     2589       -2     
- Misses          691      694       +3     
  Partials        138      138              
Impacted Files Coverage Δ
...main/java/seedu/address/commons/core/Messages.java 0.00% <ø> (ø)
src/main/java/seedu/address/model/Model.java 100.00% <ø> (ø)
...main/java/seedu/address/model/contact/Contact.java 93.20% <90.90%> (+0.42%) ⬆️
...u/address/logic/commands/contact/CMarkCommand.java 89.65% <100.00%> (-10.35%) ⬇️
...address/logic/commands/contact/CUnmarkCommand.java 100.00% <100.00%> (ø)
...edu/address/logic/commands/event/EMarkCommand.java 100.00% <100.00%> (ø)
...u/address/logic/commands/event/EUnmarkCommand.java 100.00% <100.00%> (ø)
...in/java/seedu/address/logic/parser/ParserUtil.java 98.86% <100.00%> (+0.05%) ⬆️
...dress/logic/parser/contact/CMarkCommandParser.java 100.00% <100.00%> (ø)
...ess/logic/parser/contact/CUnmarkCommandParser.java 100.00% <100.00%> (ø)
... and 8 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 2ff0fe7...d1f6bb4. Read the comment docs.

Copy link
Collaborator

@xiangjunn xiangjunn left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 287 to 288
|| (
other instanceof AddressBook // instanceof handles nulls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
|| (
other instanceof AddressBook // instanceof handles nulls
|| (other instanceof AddressBook // instanceof handles nulls

Comment on lines 261 to 296
.append("; Email: ")
.append(getEmail())
.append(getPhone() != null ? "; Phone: " + getPhone() : "") // optional
.append(getAddress() != null ? "; Address: " + getAddress() : "") // optional
.append(getZoomLink() != null ? "; Zoom Link: " + getZoomLink() : "") // optional
.append(getTelegramHandle() != null ? "; Telegram: " + getTelegramHandle() : ""); // optional
.append("; Email: ")
.append(getEmail())
.append(getPhone() != null ? "; Phone: " + getPhone() : "") // optional
.append(getAddress() != null ? "; Address: " + getAddress() : "") // optional
.append(getZoomLink() != null ? "; Zoom Link: " + getZoomLink() : "") // optional
.append(getTelegramHandle() != null ? "; Telegram: " + getTelegramHandle() : ""); // optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you changing all the indentations from 8 to 4 spaces? Java coding standard is 8 spaces for wrapped lines.

Comment on lines 175 to 176
|| (
other instanceof UniqueEventList // instanceof handles nulls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you split it like this?

tP Project Dashboard automation moved this from In progress to Review in progress Nov 7, 2021
tP Project Dashboard automation moved this from Review in progress to Reviewer approved Nov 7, 2021
@xiangjunn xiangjunn merged commit 43f3e9c into master Nov 7, 2021
tP Project Dashboard automation moved this from Reviewer approved to Done Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity.High A flaw that affects most users and causes major problems for users type.Bug Something isn't working
Development

Successfully merging this pull request may close these issues.

Mark and unmark with duplicate index bug Fix bug between emark and esort
2 participants