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

Testing instructions #134

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

JerryO3
Copy link

@JerryO3 JerryO3 commented Apr 15, 2024

Add testcases to appendix.

Please review to ensure no errors, especially for sort and pay which I am not that familiar with!

@JerryO3 JerryO3 requested review from zhekaiii and alvinnzz April 15, 2024 13:45
Copy link

@zhekaiii zhekaiii left a comment

Choose a reason for hiding this comment

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

Nothing much to nitpick on, just the birthday and the pay command thing we talked about earlier.

Comment on lines 1025 to 1026
Expected: Person list now shows the filtered list of persons sorted based on upcoming birthdays, with those
whose birth month and birth day are closest to today appearing first.

Choose a reason for hiding this comment

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

I think you can rephrase to next birthday closest to today? Because later people niao our DG then say "the birthday is yesterday so it should appear first because 1 day difference".

Copy link

@zhekaiii zhekaiii left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@zhekaiii zhekaiii merged commit 1835a78 into AY2324S2-CS2103T-T16-2:master Apr 15, 2024
3 checks passed
Copy link

@alvinnzz alvinnzz left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

3 participants