Add support for all types of monthly repeating schedules #1462
Conversation
Previously we just handled monthly schedules which repeated on a day (1-31) or 'LastDay'. Tableau Server has since added more options such as first Monday. This change catches up the interval validation to match what might be received from the server. Fixes #1358
| raise ValueError(error) | ||
| try: | ||
| if not (1 <= int(interval_value) <= 31): | ||
| error = f"Invalid monthly numeric frequency interval: {interval_value}." |
There was a problem hiding this comment.
This error message can never happen right? Its ValueError will get caught below and either swallowed or a new ValueError will be thrown. I haven't used Python much, is it common to use exceptions to control flow?
Maybe use the isinstance(interval_value, (int, float)) construct like on line 80 to detect if it's a number first.
Or if it's always a string, interval_value.isdigit()
There was a problem hiding this comment.
Well in theory I think it would fail right there if the numerical value was 32 or "32".
I agree using exceptions here is a little funky. I was trying to fit in with the methods used already.
There was a problem hiding this comment.
Using exceptions for control flow isn't uncommon in python. The except ValueError on 274 would indeed catch and consume the one raised on 273. The type check proposed by @anyoung-tableau does seem like a good alternative to let the different errors surface.
There was a problem hiding this comment.
I'll take another look at it. The challenge is that even numbers are strings by the time they land here.
There was a problem hiding this comment.
Easiest way to do it seems like explicitly listing the known allowed digits:
for value in range(1, 32):
VALID_INTERVALS.add(str(value))
| raise ValueError(error) | ||
| try: | ||
| if not (1 <= int(interval_value) <= 31): | ||
| error = f"Invalid monthly numeric frequency interval: {interval_value}." |
There was a problem hiding this comment.
Easiest way to do it seems like explicitly listing the known allowed digits:
for value in range(1, 32):
VALID_INTERVALS.add(str(value))
|
@jacalata I modified the validation and simplified by including the 1..31 range as both int or string. Looks better now and avoids that "never reachable" error path I started with. |
Previously we just handled monthly schedules which repeated on a day (1-31) or 'LastDay'. Tableau Server has since added more options such as "first Monday". This change catches up the interval validation to match what might be received from the server. Fixes #1358 * Add failing test for "monthly on first Monday" schedule * Add support for all monthly schedule variations * Unrelated fix for debug logging of API responses and add a small warning
This should fix handling all of the "monthly" schedule types now; see details in #1358 and #1369.