-
Notifications
You must be signed in to change notification settings - Fork 101
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
Modernize tests & linting #114
Conversation
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 don't have anything blocking in here, only the one question.
|
||
s.add_dependency "jekyll", ">= 2.4" | ||
all_files = `git ls-files -z`.split("\x0") | ||
s.files = all_files.grep(%r!^(lib)/!) |
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 use Dir globbing instead of git
to get the file list? There's a growing practice of folks not depending on git to get the file list for gems in order to avoid putting git on production like systems.
else | ||
Utils.deep_merge_hashes(DEFAULTS, config["jekyll-archives"]) | ||
end | ||
@config = Utils.deep_merge_hashes(DEFAULTS, config.fetch("jekyll-archives", {})) |
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.
❤️
Hash#fetch
is my jam
@@ -21,6 +21,7 @@ require "rake/testtask" | |||
Rake::TestTask.new(:test) do |test| | |||
test.libs << "lib" << "test" | |||
test.pattern = "test/**/test_*.rb" | |||
test.warning = false |
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.
What does this do? Suppress ruby warnings in the test output?
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.
Yes!
@jekyllbot: merge +dev |
No description provided.