Fix period type support for Pandas 2.2.0 #7988
Conversation
sfc-gh-lmasuch
changed the title
| // Reason is that these types are not supported by moment.js, but also they are not | ||
| // very commonly used in practice. | ||
| type SupportedPandasOffsetType = | ||
| // yearly frequency: |
There was a problem hiding this comment.
Should this comment go with "Y"?
There was a problem hiding this comment.
A and Y are both yearly frequencies
There was a problem hiding this comment.
A is the deprecated alias
| // calendar day frequency: | ||
| | "D" | ||
| // hourly frequency: | ||
| | "H" // deprecated alias |
There was a problem hiding this comment.
is "H" a deprecated alias or "h"?
Should it be:
// hourly frequency:
| "H"
// deprecated alias
"h"
?
There was a problem hiding this comment.
H is deprecated. The comments are always in the same line of the deprecated alias
| type PeriodFrequency = | ||
| | SupportedPandasOffsetType | ||
| | `${SupportedPandasOffsetType}-${string}` | ||
| type PeriodType = `period[${PeriodFrequency}]` | ||
|
|
||
| const WEEKDAY_SHORT = ["SUN", "MON", "TUE", "WED", "THU", "FRI", "SAT"] | ||
| const formatMs = (duration: number): string => |
There was a problem hiding this comment.
super nit: looks like this just adds some duration of seconds to 1970-01-01 and then formats it to that specific format.
Should the function name be addMsAndFormat or something different?
There was a problem hiding this comment.
The value we have is just a millisecond number and we want to format that number. That it gets added to 1970 is more of a detail of the formatting itself (we want it to be formatted to a full date). So I think using addMsAndFormat would be slightly more confusing.
* Fix period type support with Pandas 2.2.x * Add more comments
Describe your changes
In Pandas 2.2.0, the "A, H, T, S, L, U, and N are deprecated in favour of the aliases Y, h, min, s, ms, us, and ns."
This PR adds support for the new aliases.
Testing Plan
The old e2e tests will cover these cases. But I started a bigger improvement / refactoring related to the period type here: #7987 which will also contain refactored tests.
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.