Skip to content
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

Added a get_param_as_datetime() method to fetch query param as datetime #1053

Merged
merged 6 commits into from Jun 15, 2017

Conversation

kgritesh
Copy link
Contributor

@kgritesh kgritesh commented May 21, 2017

Solution to #1007.

@kgritesh kgritesh force-pushed the feature_add_param_as_datetime branch from 8833f94 to dd4e778 Compare May 26, 2017 10:26
@codecov
Copy link

codecov bot commented May 26, 2017

Codecov Report

Merging #1053 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1053   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        2173    2181    +8     
  Branches      309     311    +2     
======================================
+ Hits         2173    2181    +8
Impacted Files Coverage Δ
falcon/request.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 975565b...f38533a. Read the comment docs.

Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

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

Just a couple of suggestions and then I think this is ready to go. Thanks!


Args:
name (str): Parameter name, case-sensitive (e.g., 'ids').

Keyword Args:
format_string (str): String used to parse the param value
into a date. Any format recognized by strptime() is
into a datetime. Any format recognized by strptime() is
supported (default ``"%Y-%m-%d"``).
Copy link
Member

Choose a reason for hiding this comment

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

This default string needs to be updated.

req = resource.captured_req
store = {}
req.get_param_as_datetime('thedate', store=store)
assert len(store) != 0
Copy link
Member

Choose a reason for hiding this comment

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

Just to be safe, I think it would make sense to also check the value that was added to store.

@kgritesh kgritesh force-pushed the feature_add_param_as_datetime branch from 1943044 to 1f29344 Compare May 31, 2017 06:12
@kgritesh
Copy link
Contributor Author

@kgriffs fixed as per your review. Pls have a look

Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

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

Thanks, the changes you made look great. Just one more thing to look at.

def get_param_as_date(self, name, format_string='%Y-%m-%d',
required=False, store=None):
"""Return the value of a query string parameter as a date.
def get_param_as_datetime(self, name, format_string='%Y-%m-%dT%H:%M:%S',
Copy link
Member

@kgriffs kgriffs Jun 2, 2017

Choose a reason for hiding this comment

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

Upon further reflection, I think it may be better to require the Z UTC timezone designator (i.e., %Y-%m-%dT%H:%M:%SZ) in the default format. The above format string implies ISO 8601, in which case the absence of a time zone designator would indicate local time, which is ambiguous and therefore something that we should not encourage.

@jmvrbanac jmvrbanac merged commit ef03f4c into falconry:master Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants