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

Issue-230: add load_all method to Holidays namespace to preload all d… #233

Merged
merged 1 commit into from
Oct 23, 2016

Conversation

ppeble
Copy link
Member

@ppeble ppeble commented Oct 10, 2016

Fixes #230

@ppeble
Copy link
Member Author

ppeble commented Oct 10, 2016

I took the opportunity to remove all of the old comments in the main lib/holidays.rb file that were kind of out of date. I'm not a big fan of doc like this...they almost always become out of date. The code should self-document. If it doesn't we should address it.

@coveralls
Copy link

coveralls commented Oct 10, 2016

Coverage Status

Coverage increased (+0.001%) to 99.476% when pulling 2623f52 on ptrimble:issue-230 into f216eda on holidays:master.

Copy link
Member

@ttwo32 ttwo32 left a comment

Choose a reason for hiding this comment

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

Looks good to me.:)

@@ -9,44 +9,6 @@
require 'holidays/errors'
require 'holidays/load_all_definitions'

# == Region options
Copy link
Contributor

@espen espen Oct 23, 2016

Choose a reason for hiding this comment

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

are all these comments meant to be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. They were out of date in some cases and honestly I'm not a huge fan of in-code comments like this. I believe that the code should be self-documenting and, if additional doc is needed, the README should fill in the gaps.

Thoughts? Was this kind of documentation useful to you? I'm open to discussing it, although I would want to re-add it in another PR/issue so that they are up to date if we do go that route.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought maybe it was a mistake since it was committed with this patch. I guess it's covered in the readme. I think it's fine for this project with not so many methods, for larger projects like Rails I think inline commenting is useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. I'm not as anti-comment as some of my coworkers (who refuse to allow any of them, ever). I bet that you and I are probably pretty close on what we would find 'acceptable'.

In this case I'll merge this and think about the best way to document for future developers.

Thanks!

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.

4 participants