Fix storage seconds #1888
Fix storage seconds #1888
Conversation
|
Hi @oakeyc, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
|
Still have Pylint errors |
| try: | ||
| return datetime.strptime(string, form) | ||
| except ValueError: # checks next format | ||
| pass |
There was a problem hiding this comment.
Using continue here instead of pass would make the logic more clear and remove the need for the comment. Ultimately they do the same thing in this case.
| return datetime.strptime(string, form) | ||
| except ValueError: # checks next format | ||
| pass | ||
| raise ValueError("Not Valid Date Format") |
There was a problem hiding this comment.
You should print out what didn't have a valid date format (the bad input). Otherwise the user won't know which one was screwed up.
| '%Y-%m-%dT%HZ', '%Y-%m-%d'] | ||
| for form in accepted_date_formats: | ||
| try: | ||
| return datetime.strptime(string, form).strftime(form) |
There was a problem hiding this comment.
It seems like this could be refactored to leverage the existing datetime_type method and avoid duplicating the logic here. You could merge these into one method with a parameter to toggle the behavior:
get_datetime_type_method(to_string=False):
def datetime_type(string):
# stuff goes here
if to_string:
return datetime.strptime(string, form).strftime(form)
else:
return datetime.strptime(string, form)
return datetime_type
| date_format = '%Y-%m-%dT%H:%MZ' | ||
| return datetime.strptime(string, date_format) | ||
| """ Validates UTC datettime in accepted format. Examples: 2017-12-31T01:11:59Z, | ||
| 2017-12-31T01:11Z or 2017-12-31T01Z or 2017-12-31 """ |
There was a problem hiding this comment.
All looks fine exception the format: "2017-12-31T01Z" looks a bit unconventional. It is not one of the format accpetable to Azure Restful API
@tjprescott thoughts?
There was a problem hiding this comment.
If it isn't acceptable to the API then we shouldn't allow it. However, this is in the KeyVault package. Did you mean to change this?
| return datetime.strptime(string, date_format) | ||
| def get_datetime_type(to_string): | ||
| """ Validates UTC datetime. Examples of accepted forms: | ||
| 2017-12-31T01:11:59Z,2017-12-31T01:11Z or 2017-12-31T01Z or 2017-12-31 """ |
There was a problem hiding this comment.
You don't need this doc string.
| ''' Validates UTC datettime in format '%Y-%m-%d\'T\'%H:%M\'Z\''. ''' | ||
| date_format = '%Y-%m-%dT%H:%MZ' | ||
| return datetime.strptime(string, date_format) | ||
| def get_datetime_type(to_string): |
There was a problem hiding this comment.
This should be a keyword argument. So when you call this method, you'd have get_datetime_type(to_string=False) etc.
Having get_datetime_type(False) looks odd as I can't tell what False means without having to look at the method definition.
Also, then if a better keyword argument that describes things than to_string can be thought of, this should be changed. Right now, I don't know what to_string actually means so I don't have any suggestions for a better replacement. :)
| return datetime.strptime(string, form) | ||
| except ValueError: | ||
| continue | ||
| raise ValueError("Input not of valid format. Valid format: 2000-12-31T12:59:59Z") |
There was a problem hiding this comment.
You still need to put what the input was that is invalid, not just an example format (though that is good). So it would be:
raise ValueError("Input '{}' is not valid. ... ".format(string))
Fixes #1560, fixes #1563