Feat/date range option for all readers #83
Conversation
Based on the existing function for reach & freq reports in DBM, add more date ranges: YESTERDAY, LAST_7_DAYS, LAST_90_DAYS
Now possible to specify a date range instead of a date start + date stop. Date ranges will be translated into date start and date stop automatically in the code.
Now 1 list that is used across all readers so that if we want to add a new date range, we don't have to update all readers
There was a problem hiding this comment.
Pretty neat. 👍
The next steps would be to add this in a util or another folder so that we can use this logic for other readers right ? Like just build the logic using start-date, end-date and date-range as optional parameters and deal with all the cases possible. Let me know what you think.
| self.ingestion_tracker = [] | ||
| self.node_values = {} | ||
|
|
||
| def build_date_range(self): | ||
| return f"{self.start_date.strftime(DATEFORMAT)}/{self.end_date.strftime(DATEFORMAT)}" | ||
| if self.start_date is not None and self.end_date is not None and self.date_range is None: |
There was a problem hiding this comment.
So do we raise an error in case of both start/end dates and date range are given by the user? What about checking if end-start = date range in case all parameters are given ?
Btw seems like the same error would occur if only one of the start/end date is None and the other one is not.
There was a problem hiding this comment.
Yes correct. This is documented in the issue #67. We do this to avoid conflicts between a date range and 2 dates. For example, if the user set a date start and a date stop but also a date range to LAST_7_DAYS, what would we do? We would have to give the priority to one parameter on the other but it would be implicit.
Your suggestion would work but it would make things more difficult imo, so I prefer letting things like that.
There was a problem hiding this comment.
Yes I agree, this is enough for now. The value/time ratio for the feature I mentioned is too low to consider it for now.
Yes, part of the code is already in a utils file. The only thing we have to duplicate for now is the input validation but this is the subject of the other PR on date management: #67 |
There was a problem hiding this comment.
Thank you Benoit, you implemented this feature very smartly! The logic can be easily replicated on other readers. I also agree with the way you chose to handle exceptions: the rest of the job will be done as part of the second PR about dates.
I was just concerned on how your feature would interact with the little nuance of the Adobe API about end dates (we have to increment them by one day to include them in the requested time period), but I saw that you already handled it. All good then! 👌
docs/source/readers.rst
Outdated
| @@ -128,6 +128,7 @@ Options Definition | |||
| ``--adobe-2-0-metric`` Metric to include in the report | |||
| ``--adobe-2-0-start-date`` Start date of the period to request (format: YYYY-MM-DD) | |||
| ``--adobe-2-0-end-date`` Start date of the period to request (format: YYYY-MM-DD) | |||
| ``--adobe-2-0-date-range`` Date range. By default, not available in Adobe, so choose among NCK default values. | |||
There was a problem hiding this comment.
Maybe we should just document somewhere what are these NCK default values (once for all readers) ?
There was a problem hiding this comment.
Yes you are right. For now, as it is implemented in only 1 reader, I've added them directly in the doc of this first reader. Once the feature extended to other readers, we will refactor the doc.
Issue
My PR addresses the Issue #82
Description
This PR brings the skeleton for default date ranges. This PR only implements the feature in one reader: adobe v2 but it will be very easy to add this feature to other readers and I can take care of that.
So, now you can use the
--adobe-2-0-date-range YESTERDAYoption for example to test the feature.Tests
My PR adds test for the logic of date start and date stop generation.
Documentation