-
Notifications
You must be signed in to change notification settings - Fork 40
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
Include method documentation for datetime #44
Conversation
* replaced every occurence of 'netcdftime' with 'cftime' * will break tests until github/travis/appveyor names are updated
Conflicts: cftime/_cftime.pyx setup.py test/test_cftime.py
* Previously datetime.timetuple() returned a tuple, now it returns a time.struct_time, the same as a built-in datetime would. * Docstrings were not present for strftime or timetuple. They have been added based on the docstrings of the corresponding built-in datetime methods. * Autodoc settings have been changed to include inheritance information for classes.
cftime/_cftime.pyx
Outdated
@@ -1533,12 +1533,18 @@ Gregorial calendar. | |||
return '%Y-%m-%d %H:%M:%S' | |||
|
|||
def strftime(self, format=None): | |||
""" | |||
Return a string representing the date, controlled by an explicit format | |||
string. Format codes referring to hours, minutes or seconds will see 0 |
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 part about 0 values doesn't seem correct, nor should it given that it's a datetime-alike class. Am I missing something?
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 supposedly should, given I copied that docstring directly from the datetime documentation.
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.
I think it's referring to the fact that if the hours, minutes, or seconds are zero, they will be represented as zero values rather than omitted?
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.
That's the docstring for date
, not datetime
. It should read:
Return a string representing the date and time, controlled by an explicit format string. For a complete list of formatting directives, see section strftime() and strptime() Behavior.
https://docs.python.org/2/library/datetime.html#datetime.datetime.strftime
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're right. I made the same mistake when replying here! I'll fix it tomorrow.
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.
Should be fixed now.
cftime/_cftime.pyx
Outdated
Return a time.struct_time such as returned by time.localtime(). | ||
The hours, minutes and seconds are 0, and the DST flag is -1. | ||
d.timetuple() is equivalent to time.struct_time((d.year, d.month, d.day, | ||
0, 0, 0, d.weekday(), yday, -1)), where yday is the day number within |
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.
Here too
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.
Corrected. I kept "The DST flag is -1" since that's true for the cftime implementation.
cftime/_cftime.pyx
Outdated
""" | ||
Return a string representing the date, controlled by an explicit format | ||
string. For a complete list of formatting directives, see section | ||
strftime() and strptime() Behavior in the base Python documentation. |
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.
nit: behavior should be lower case
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.
Actually it is the title of a section of documentation and should be uppercase. Also italicized but that's not possible. Maybe quotation marks?
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.
oh, I see. Quotes would work here.
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.
Fixed.
@dopplershift - anything left to do here? |
Fixes #42 and #43.