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

[#1990] Move TimeUtil ParseException throwing to ArgsParser::parse method #2075

Merged
merged 9 commits into from
Dec 30, 2023
Merged

[#1990] Move TimeUtil ParseException throwing to ArgsParser::parse method #2075

merged 9 commits into from
Dec 30, 2023

Conversation

nseah21
Copy link
Contributor

@nseah21 nseah21 commented Dec 23, 2023 •
edited by chan-j-d
Loading

Fixes #1990

Proposed commit message

ArgsParser::parse uses the methods verifyDatesRangeIsCorrect
and verifySinceDateIsValid from TimeUtil to throw a 
ParseException if either of the checks fail. The throwing 
of an exception in a utility method is a side effect
that makes the methods less reusable.

Let's move the throwing of ParseException from TimeUtil 
into ArgsParser.

Other information

NA

MarcusTXK
MarcusTXK requested changes Dec 23, 2023
View reviewed changes
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

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

Great job, just a small fix and we can merge it in.

src/main/java/reposense/parser/ArgsParser.java Outdated
Comment on lines 427 to 433
if (sinceDate.compareTo(untilDate) > 0) {
throw new ParseException(MESSAGE_SINCE_DATE_LATER_THAN_UNTIL_DATE);
}

if (sinceDate.compareTo(currentDate) > 0) {
throw new ParseException(MESSAGE_SINCE_DATE_LATER_THAN_TODAY_DATE);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some test coverage for the two new if blocks added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @MarcusTXK, I've added the tests for ArgParser. The PR is ready for review.

MarcusTXK
MarcusTXK approved these changes Dec 26, 2023
View reviewed changes
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

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

LGTM, nice job with the refactoring.

@MarcusTXK MarcusTXK requested a review from a team December 26, 2023 14:11
ckcherry23
ckcherry23 approved these changes Dec 28, 2023
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chan-j-d chan-j-d merged commit d8ce443 into reposense:master Dec 30, 2023
10 checks passed
@github-actions GitHub Actions
Copy link
Contributor

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcusTXK MarcusTXK MarcusTXK approved these changes

@ckcherry23 ckcherry23 ckcherry23 approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move TimeUtil ParseException throwing to ArgsParser::parse method
4 participants
@nseah21 @MarcusTXK @ckcherry23 @chan-j-d