Stats Review: Add types to represent date period, granularity, presets, and more (P4) - #24716
Stats Review: Add types to represent date period, granularity, presets, and more (P4)#24716kean wants to merge 1 commit into
Conversation
kean
changed the title
Generated by 🚫 Danger |
|
| case .month: components.month ?? 1 | ||
| case .quarter: components.quarter ?? 1 | ||
| case .year: components.year ?? 1 | ||
| default: 1 |
There was a problem hiding this comment.
Replace the switch statement with components.value(for: component) ?? 1?
| /// - direction: Whether to navigate forward (.forward) or backward (.backward) | ||
| /// - component: The calendar component to use for navigation | ||
| /// - Returns: A new date interval representing the navigated period | ||
| func navigate(_ interval: DateInterval, direction: NavigationDirection, component: Calendar.Component) -> DateInterval { |
There was a problem hiding this comment.
I don't get the meaning of the component parameter.
Take one of the unit tests as an example:
// 7-day period should navigate by 7 days
(Date("2025-01-08T00:00:00-03:00"), Date("2025-01-15T00:00:00-03:00"), Calendar.Component.day, ...)
// Using above test arguments makes call to
navigate(
DateInterval(start: "2025-01-08", end: "2025-01-15"),
direction: .backwards,
component: .day
)
Why passing .day as the component argument? Is the component argument even necessary?
The navigate function calculates the immediate previous or next period of a given date interval. It feels like only the first two arguments are needed.
There was a problem hiding this comment.
I just saw the determineNavigationComponent function below. Is the component argument here always determineNavigationComponent(interval) in practice? If that's the case, we can omit the component and call determineNavigationComponent internally within the navigate function?
There was a problem hiding this comment.
Is the component argument even necessary?
The component is needed because the following are two different scenarios:
// Returns `DateInterval(start: "2025-05-31", end: "2025-06-30")` as it offsets by "31 days" (number of days in July)
navigate(
DateInterval(start: "2025-07-01", end: "2025-07-31"),
direction: .backwards,
component: .day
)
// Returns `DateInterval(start: "2025-06-01", end: "2025-06-30")` as it offsets by "1 month"
navigate(
DateInterval(start: "2025-07-01", end: "2025-07-31"),
direction: .backwards,
component: .month
)
I wanted to keep func navigate flexible and simple.
Is the component argument here always determineNavigationComponent(interval) in practice?
When you select a present (DateRangePreset), the component is based on the preset. The only component determineNavigationComponent currently doesn't support is quarter. I added it in #24760. I'm not sure if I want to use determineNavigationComponent automatically just yet – I might be missing something.
| } | ||
|
|
||
| /// Determines if navigation is allowed in the specified direction | ||
| func canNavigate(_ interval: DateInterval, direction: NavigationDirection, minYear: Int = 2000, now: Date = .now) -> Bool { |
There was a problem hiding this comment.
Can this function be implemented like this?
let minYearsAgo = ...
let validInterval = DateInteravl(start: minYearsAgo, end: now)
let dest = navigate(interval, direction)
return validInterval.intersects(dest)There was a problem hiding this comment.
I'd probably keep the current implementation for now. It seems a bit more direct.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function already exists in WordPressUI.
There was a problem hiding this comment.
Turns out it was unused already – removed.
| } | ||
|
|
||
| func string(from interval: DateInterval, now: Date = Date()) -> String { | ||
| var calendar = Calendar.current |
There was a problem hiding this comment.
This is an existing issue with the app. But I thought I'd mention it anyways. We should not use Calendar.current to calculate dates. What the app needs almost 100% of the time is the Gregorian calendar, which Calendar.current may not be, because users may set their iPhone calendar to be other calendars. For example, many people in China set their phone to use the Chinese Calendar.
There was a problem hiding this comment.
3 participants
Add the following types and respective unit tests:
DateIntervalPresetandCalendar+Presetsto represent the presents likeLast 7 DaysDateRangeComparisonPeriodto represent a comparison period (last period or same period last year)Calendar+Navigationto implement "backward" and "forward" actionsStatsDateFormatterandStatsDateRangeFormatterto format individual dates and date periods (see unit tests and docs)StatsValueFormatterto format values for metrics (SiteMetric)Alternative considered: using NaiveDate or date components to represent date periods. I tried it, but it turned out completely impractical as you still needed to convert to
Dateand useCalendarfor all manipulations (which there are many needed). So instead, it usesDatewith a site timezone. The site time zone is used throughout the module.