-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Time#days_in_year returns the no of days in a given year #5163
Conversation
LGTM 👍 ? |
src/time.cr
Outdated
@@ -398,6 +398,11 @@ struct Time | |||
days[month] | |||
end | |||
|
|||
# Returns how many days are there in a year |
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.
Can we change description/comment like this?
# Returns number of days in *year*
#
# ```
# Time.days_in_year(1990) # => 365
# Time.days_in_year(2004) # => 366
# ```
That way, year
is clearly identified as argument and the example provides a nice touch 😄
Thank you! ❤️ ❤️ ❤️
Ref: https://crystal-lang.org/docs/conventions/documenting_code.html
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.
There should be a .
at the end of the doc sentence.
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.
@luislavena thanks for the link. I wasn't aware of that, changing the code to such format.
PS: There are similar methods without such code examples, would a PR with such doc changes be welcomed 😄 would love to contribute like that.
src/time.cr
Outdated
@@ -398,6 +398,11 @@ struct Time | |||
days[month] | |||
end | |||
|
|||
# Returns how many days are there in a year | |||
def self.days_in_year(year : Int) : Int32 | |||
days_in_month(year, 2) + 337 |
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.
A simpler implementation and more explicit: leap_year?(year) ? 366 : 365
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 agree and also faster. I will modify the code for that.
49e3177
to
a9a17e0
Compare
@luislavena & @straight-shoota PR has been updated. |
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.
👍
Definitely @coderhs!!! All the contributions that introduce better and easier to use documentation are welcome! 😄 Thank you! |
@luislavena have added code example to |
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.
Thank you!
In reference to issue #5157