-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#634] Support defining sinceDate, untilDate and period in report config #1501
Conversation
The new report-config.json allows for the configuration of the report generation. However, only the report title can be configured at this time. Let's expand the functionality of the report config file by allowing sinceDate, untilDate and period to be defined as well. If the CLI arguments were to contain the same settings as those in the report config, then the CLI arguments will take precedence.
@dcshzj can you create smaller PRs for these changes. And then after merging those PRs we can go ahead with merging this one. Always remember, smaller neat PRs are preferred as a single PR should bring about only a single change in the repository. This helps in keeping track and reviewing the PR more efficiently. |
@Tejas2805 I have split them to #1534 and #1535. I left this PR intact and will update if those PRs are merged. |
@Tejas2805 The requisite PRs have been merged. |
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.
Left few comments for implementation and logic. Will review tests once this is sorted.
|
||
<box type="info" seamless> | ||
|
||
* `sinceDate`, `untilDate` and `period` cannot be declared together and will be ignored if all 3 configuration settings are present in the report configuration. |
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.
} else if (reportConfig.getUntilDate().isPresent()) { | ||
tempUntilDate = reportConfig.getUntilDate(); | ||
} | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @Tejas2805 I still can't figure out how to better write this part of the code. My train of thought for this is:
- Check if all 3 arguments are provided (either from report config, from CLI, or from both). If yes, we ignore all of them and take only the CLI arguments.
- For each of the 3 arguments, if it is given in both the CLI and in the report config, we give the user a warning.
- Afterwards, we take only the CLI arguments.
- We then process the actual since and until dates, based on the processed information. If both since and until dates are provided (which only happens in CLI), the period is ignored without warning.
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.
|
||
* `sinceDate`, `untilDate` and `period` cannot be declared together and will be ignored if all 3 configuration settings are present in the report configuration. | ||
* CLI arguments takes precedence over the values provided in this report configuration. If the same configuration is set as a CLI argument as well as in the report configuration, the value in the report configuration is ignored. | ||
* Likewise, if there is a conflict, such as `sinceDate`, `untilDate` and `period` being declared together even though some of them were declared in the report configuration, only the CLI arguments will be taken and the report configuration will be completely ignored. |
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 if all 3 are provided in the report config itself and nothing is provided as CLI arguments?
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.
Similar to the first part, all 3 will be ignored, as if the user tried to specify all 3 in the CLI arguments instead of the report config.
@Tejas2805 Left some comments on your input |
@Tejas2805 Any comments? |
@Tejas2805 Can I please have your comments on this? |
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.
Sorry for the late reply. Left a comment.
} else if (reportConfig.getUntilDate().isPresent()) { | ||
tempUntilDate = reportConfig.getUntilDate(); | ||
} | ||
|
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 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.
Closing this 4-month old PR. |
Fixes #634.
Part of #1360.
Summary of changes
report-config.json
file now supports settingsinceDate
,untilDate
andperiod
. The equivalent CLI arguments are--since
,--until
and--period
respectively. (this PR)