Implement roll_monthday, simplify SemiMonthOffset by jbrockmendel · Pull Request #18762 · pandas-dev/pandas (original) (raw)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. nice tests!

only minor nitpick and can be fixed later (or here if you want). is that the signature for shift_months and the roll_* use slightly different terms, e.g.

shift_months(datetime stamp, int months)

while

roll_monthday(datetime other, int n)

so maybe make this consistent by making the args
datetime value, int n (roll_convention has an int first arg so can't really call this stamp)