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

Time#days_in_year returns the no of days in a given year #5163

Merged
merged 3 commits into from Oct 25, 2017

Conversation

coderhs
Copy link
Contributor

@coderhs coderhs commented Oct 22, 2017

In reference to issue #5157

@sdogruyol
Copy link
Member

LGTM 👍 ?

src/time.cr Outdated
@@ -398,6 +398,11 @@ struct Time
days[month]
end

# Returns how many days are there in a year
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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.

@coderhs
Copy link
Contributor Author

coderhs commented Oct 23, 2017

@luislavena & @straight-shoota PR has been updated.

Copy link
Contributor

@luislavena luislavena left a comment

Choose a reason for hiding this comment

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

👍

@luislavena
Copy link
Contributor

There are similar methods without such code examples, would a PR with such doc changes be welcomed 😄 would love to contribute like that.

Definitely @coderhs!!! All the contributions that introduce better and easier to use documentation are welcome! 😄

Thank you!
❤️ ❤️ ❤️

@coderhs
Copy link
Contributor Author

coderhs commented Oct 25, 2017

@luislavena have added code example to days_in_month and a test for that, have added the test for that method along with this PR, as mentioned in issue #5157

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Thank you!

@asterite asterite merged commit c8ee444 into crystal-lang:master Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants