[#634] Support defining sinceDate, untilDate and period in report config #1501
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| { | ||
| "title": "CS2103 RepoSense Report", | ||
| "sinceDate": "23/01/2021", | ||
| "untilDate": "23/03/2021", | ||
| "period": "30d" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,85 @@ | ||
| package reposense.model; | ||
|
|
||
| import java.util.Date; | ||
| import java.util.Optional; | ||
| import java.util.logging.Logger; | ||
|
|
||
| import reposense.parser.ParseException; | ||
| import reposense.parser.PeriodArgumentType; | ||
| import reposense.parser.SinceDateArgumentType; | ||
| import reposense.system.LogsManager; | ||
| import reposense.util.TimeUtil; | ||
|
|
||
| /** | ||
| * Represents configuration information from JSON config file for generated report. | ||
| */ | ||
| public class ReportConfiguration { | ||
| private static final Logger logger = LogsManager.getLogger(ReportConfiguration.class); | ||
| private static final String DEFAULT_TITLE = "RepoSense Report"; | ||
| private static final String MESSAGE_INVALID_DATE_PROVIDED = "Invalid date provided in report config: %s."; | ||
| private static final String MESSAGE_IGNORING_REPORT_CONFIG = "Ignoring the report config provided."; | ||
| private String title; | ||
| private String sinceDate; | ||
| private String untilDate; | ||
| private String period; | ||
|
|
||
| public String getTitle() { | ||
| if (this.title == null) { | ||
| return DEFAULT_TITLE; | ||
| } | ||
|
|
||
| return this.title; | ||
| } | ||
|
|
||
| public Optional<Date> getSinceDate() { | ||
| if (SinceDateArgumentType.FIRST_COMMIT_DATE_SHORTHAND.equals(this.sinceDate)) { | ||
| return Optional.of(SinceDateArgumentType.ARBITRARY_FIRST_COMMIT_DATE); | ||
| } | ||
|
|
||
| return this.getDate(this.sinceDate, "00:00:00"); | ||
| } | ||
|
|
||
| public Optional<Date> getUntilDate() { | ||
| return this.getDate(this.untilDate, "23:59:59"); | ||
| } | ||
|
|
||
| public Optional<Integer> getPeriod() { | ||
| if (this.period == null) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| try { | ||
| return PeriodArgumentType.parse(this.period); | ||
| } catch (ParseException pe) { | ||
| logger.warning(pe.getMessage() + " " + MESSAGE_IGNORING_REPORT_CONFIG); | ||
| return Optional.empty(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * This function resets the values of the {@code sinceDate}, {@code untilDate} and {@code period} to null, such that | ||
| * RepoSense ignores the configuration provided in the report config provided. | ||
| */ | ||
| public void ignoreSinceUntilDateAndPeriod() { | ||
| this.sinceDate = null; | ||
| this.untilDate = null; | ||
| this.period = null; | ||
| } | ||
|
|
||
| /** | ||
| * This function parses the given {@code date} String into a Date object. | ||
| */ | ||
| private Optional<Date> getDate(String date, String time) { | ||
| if (date == null) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| try { | ||
| String value = TimeUtil.extractDate(date); | ||
| return Optional.of(TimeUtil.parseDate(value + " " + time)); | ||
| } catch (java.text.ParseException pe) { | ||
| logger.warning(String.format(MESSAGE_INVALID_DATE_PROVIDED, date) + " " + MESSAGE_IGNORING_REPORT_CONFIG); | ||
| return Optional.empty(); | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,10 +66,6 @@ public class ArgsParser { | |
| private static final String PROGRAM_DESCRIPTION = | ||
| "RepoSense is a contribution analysis tool for Git repositories."; | ||
| private static final String MESSAGE_HEADER_MUTEX = "mutual exclusive arguments"; | ||
| private static final String MESSAGE_HAVE_SINCE_DATE_UNTIL_DATE_AND_PERIOD = | ||
| "\"Since Date\", \"Until Date\", and \"Period\" cannot be applied together."; | ||
| private static final String MESSAGE_USING_DEFAULT_CONFIG_PATH = | ||
|
|
@@ -243,6 +239,9 @@ public static CliArguments parse(String[] args) throws HelpScreenException, Pars | |
| Namespace results = parser.parseArgs(args); | ||
| Date sinceDate; | ||
| Date untilDate; | ||
| Optional<Date> tempSinceDate = Optional.empty(); | ||
| Optional<Date> tempUntilDate = Optional.empty(); | ||
| Optional<Integer> tempPeriod = Optional.empty(); | ||
|
|
||
| Path configFolderPath = results.get(CONFIG_FLAGS[0]); | ||
| Path reportFolderPath = results.get(VIEW_FLAGS[0]); | ||
|
|
@@ -274,30 +273,67 @@ public static CliArguments parse(String[] args) throws HelpScreenException, Pars | |
| } | ||
| } | ||
|
|
||
| // CLI arguments are treated with the highest priority and will throw an Exception if a conflict occurs | ||
| if (cliSinceDate.isPresent() && cliUntilDate.isPresent() && cliPeriod.isPresent()) { | ||
| throw new ParseException(MESSAGE_HAVE_SINCE_DATE_UNTIL_DATE_AND_PERIOD); | ||
| } | ||
| int numCloningThreads = results.get(CLONING_THREADS_FLAG[0]); | ||
| int numAnalysisThreads = results.get(ANALYSIS_THREADS_FLAG[0]); | ||
|
|
||
| boolean isSinceDateProvided = cliSinceDate.isPresent() || reportConfig.getSinceDate().isPresent(); | ||
| boolean isUntilDateProvided = cliUntilDate.isPresent() || reportConfig.getUntilDate().isPresent(); | ||
| boolean isPeriodProvided = cliPeriod.isPresent() || reportConfig.getPeriod().isPresent(); | ||
|
|
||
| // Report config is treated with less priority and will be ignored if there is a conflict | ||
| if (isSinceDateProvided && isUntilDateProvided && isPeriodProvided) { | ||
| logger.warning(MESSAGE_IGNORING_REPORT_SINCE_DATE_UNTIL_DATE_AND_PERIOD); | ||
| reportConfig.ignoreSinceUntilDateAndPeriod(); | ||
| isSinceDateProvided = cliSinceDate.isPresent(); | ||
| isUntilDateProvided = cliUntilDate.isPresent(); | ||
| isPeriodProvided = cliPeriod.isPresent(); | ||
| } | ||
| if (cliSinceDate.isPresent() && reportConfig.getSinceDate().isPresent()) { | ||
| logger.warning(MESSAGE_IGNORING_REPORT_CONFIG_SINCE_DATE); | ||
| } | ||
| if (cliUntilDate.isPresent() && reportConfig.getUntilDate().isPresent()) { | ||
| logger.warning(MESSAGE_IGNORING_REPORT_CONFIG_UNTIL_DATE); | ||
| } | ||
| if (cliPeriod.isPresent() && reportConfig.getPeriod().isPresent()) { | ||
| logger.warning(MESSAGE_IGNORING_REPORT_CONFIG_PERIOD); | ||
| } | ||
|
|
||
| // Set the values to those provided by the CLI arguments first, otherwise take from report config | ||
| if (cliPeriod.isPresent()) { | ||
| tempPeriod = cliPeriod; | ||
| } else if (reportConfig.getPeriod().isPresent()) { | ||
| tempPeriod = reportConfig.getPeriod(); | ||
| } | ||
| if (cliSinceDate.isPresent()) { | ||
| tempSinceDate = cliSinceDate; | ||
| } else if (reportConfig.getSinceDate().isPresent()) { | ||
| tempSinceDate = reportConfig.getSinceDate(); | ||
| } | ||
| if (cliUntilDate.isPresent()) { | ||
| tempUntilDate = cliUntilDate; | ||
| } else if (reportConfig.getUntilDate().isPresent()) { | ||
| tempUntilDate = reportConfig.getUntilDate(); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Find the whole logic above a bit cumbersome. There is a better way to avoid a few of the checks by writing the if conditions in a clever manner. I will leave that to you to figure out. There was a problem hiding this comment. I thought of the following, not sure if that is what you are looking for? tempPeriod = Optional.ofNullable(cliPeriod.orElse(reportConfig.getPeriod().orElse(null)));
tempSinceDate = Optional.ofNullable(cliSinceDate.orElse(reportConfig.getSinceDate().orElse(null)));
tempUntilDate = Optional.ofNullable(cliUntilDate.orElse(reportConfig.getUntilDate().orElse(null)));There was a problem hiding this comment. I meant for the whole page in general. Not just the last three lines. Cause I can see you see checking if arguments are present or not 2/3 times. So I didn't like that part of the logic. Can try to make sure that you check these only once and make it work. There was a problem hiding this comment. Sorry @Tejas2805 I still can't figure out how to better write this part of the code. My train of thought for this is:
I feel that they are all checking different types of conditions, so I can't wrap my head around how to better craft this section. |
||
| Date currentDate = TimeUtil.getCurrentDate(zoneId); | ||
|
|
||
| if (isSinceDateProvided) { | ||
| sinceDate = TimeUtil.getZonedSinceDate(tempSinceDate.get(), zoneId); | ||
| } else { | ||
| sinceDate = isPeriodProvided | ||
| ? TimeUtil.getDateMinusNDays(tempUntilDate, zoneId, tempPeriod.get()) | ||
| : TimeUtil.getDateMinusAMonth(tempUntilDate, zoneId); | ||
| } | ||
|
|
||
| if (isUntilDateProvided) { | ||
| untilDate = TimeUtil.getZonedUntilDate(tempUntilDate.get(), zoneId); | ||
| } else { | ||
| untilDate = (isSinceDateProvided && isPeriodProvided) | ||
| ? TimeUtil.getDatePlusNDays(tempSinceDate, zoneId, tempPeriod.get()) | ||
| : currentDate; | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for ignoring all 3? Why not just take since and until in this case, assuming nothing provided in CLI arguments. Don't do changes as of now, just explain the intuition behind this logic for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the logic originally present for CLI arguments, whereby if all 3 arguments are provided we will throw an exception. In this case, because it is in report config, I chose to ignore instead of throwing an exception.