-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
When merging, specify a better commit message that "Fix storage seconds".
Maybe something like "[Storage] Allow up to the second precision".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor changes.
try: | ||
return datetime.strptime(string, form) | ||
except ValueError: # checks next format | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message should still be improved and a question about KeyVault.
Fixes #1560, fixes #1563