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

format_datetime: using 'S...S' format is incorrect #353

Closed
wants to merge 2 commits into from

Conversation

shubh1m
Copy link
Contributor

@shubh1m shubh1m commented Feb 22, 2016

Fixes #350

@akx
Copy link
Member

akx commented Feb 24, 2016

This is missing test cases.

You would need to prove that the earlier behavior is wrong and that this patch fixes it. :)

@shubh1m
Copy link
Contributor Author

shubh1m commented Feb 24, 2016

@akx Oh thanks!! I will see to it. 😄

@codecov-io
Copy link

Current coverage is 89.66%

Merging #353 into master will not affect coverage as of 2f9c97c

@@            master    #353   diff @@
======================================
  Files           23      23       
  Stmts         3794    3794       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit           3402    3402       
  Partial          0       0       
  Missed         392     392       

Review entire Coverage Diff as of 2f9c97c

Powered by Codecov. Updated on successful CI builds.

@akx
Copy link
Member

akx commented Feb 26, 2016

Hey @iamshubh22, okay, so now you have a test case, but I'd still like to see some sort of reasoning (maybe in the form of a reference to the particular piece of specification?) that explains why this new behavior is correct :)

In addition, a couple more test cases wouldn't go amiss, explaining the different cases that the newly implemented behavior covers.

return self.format(round(float('.%s' % value), num) * 10**num, num)
value = self.value.microsecond / 1000000
return self.format(round(value, num) * 10**num, num)
""" Returns fractional seconds. Converts microseconds received to the \
Copy link
Member

Choose a reason for hiding this comment

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

The docstring for a function needs to be before the code, not after. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants