add dotfiles support #498
add dotfiles support #498
Conversation
mpapis
force-pushed
the
feature/dotfiles_gh_492
branch
from
4f0595b
to
96628bf
Compare
| # | ||
| # @raise [MaxSymlinkDepthExceededError] if too many indirections are | ||
| # encountered while resolving symlinks | ||
| # | ||
| # @raise [UnsupportedFileTypeError] if a file of an unsupported type is | ||
| # detected (something other than file, directory or link) | ||
| def all_files_in(dir_name, recursion_limit = 10) |
There was a problem hiding this comment.
The addition of the new extra_files param makes it not backwards compatible. Could this be converted into a params = {} defaulting to []?
There was a problem hiding this comment.
https://github.com/nanoc/nanoc/blob/feature/dotfiles_gh_492/lib/nanoc/extra/filesystem_tools.rb#L7 :
# @api privatewouldn't that mean we do not have to care about backwards compatibility? ... not that I can't do that change, just curious.
There was a problem hiding this comment.
You’re right. I didn’t spot the @api private. So, never mind :)
|
More in-depth review coming soon! |
| when String | ||
| patterns << "#{dir_name}/#{extra_files}" | ||
| when Array | ||
| patterns += extra_files.map { |extra_file| "#{dir_name}/#{extra_file}" } |
There was a problem hiding this comment.
For consistency, I’d rather have this be
extra_files.each { |extra_file| patterns << "#{dir_name}/#{extra_file}" }There was a problem hiding this comment.
taking into account removing the default maybe it would be better to change previous line:
patterns << "#{dir_name}/#{extra_files}"to
patterns += [ "#{dir_name}/#{extra_files}" ]?
the each << pattern feels java like (in the monkeys typing form), there are good things in java, this is not one of them.
There was a problem hiding this comment.
I prefer #<< in this case, since it modifies the array rather than creating a modified duplicate. Since #all_files_and_dirs_in is the method in which the array is constructed, modifying the array in-place is good.
It’s also more compact, and arguably easier to read.
There was a problem hiding this comment.
I think it might be slower ... but I will do the fix in the evening
There was a problem hiding this comment.
Modifying the array in-place is definitely faster:
require 'benchmark'
Benchmark.bmbm do |x|
x.report('#<<') do
a = []
50_000.times { a << 1 }
end
x.report('#+=') do
a = []
50_000.times { a += [1] }
end
end user system total real
#<< 0.010000 0.000000 0.010000 ( 0.008376)
#+= 2.310000 3.880000 6.190000 ( 6.208789)
Not that it matters much in this case, but there’s no reason to use += over << here.
There was a problem hiding this comment.
that's surprising, will have to report a bug for ruby
There was a problem hiding this comment.
It’s not a bug. The + form creates a new array by copying the original array and adding the new element. Copying is slow and creates lots of temporary objects that stress the GC.
There was a problem hiding this comment.
well I was expecting += to behave as concat => jruby/jruby#2256
There was a problem hiding this comment.
I prefer #concat:
patterns.concat(extra_files.map { |extra_file| "#{dir_name}/#{extra_file}" })
|
You’ll probably have to rebase against |
mpapis
force-pushed
the
feature/dotfiles_gh_492
branch
from
96628bf
to
0859e20
Compare
|
I think the remaining bug will be fixed with #502 |
|
by bug I meant test failures |
.htaccess and .htpasswd were not handled by any data-source
mpapis
force-pushed
the
feature/dotfiles_gh_492
branch
from
7e8d183
to
1782d70
Compare
| when String | ||
| patterns << "#{dir_name}/#{extra_files}" | ||
| when Array | ||
| patterns.concat(extra_files.map { |extra_file| "#{dir_name}/#{extra_file}" }) |
There was a problem hiding this comment.
now, is << and concat ok, or should I change << to concat([...])?
There was a problem hiding this comment.
Yup, that’s OK. It doesn’t make sense to use #concat when appending only a single element.
|
and the tests pass after rebasing on top of merged #502 |
|
👍 |
#492 with whitelist support