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

add dotfiles support #498

Merged
merged 8 commits into from Dec 13, 2014
Merged

add dotfiles support #498

merged 8 commits into from Dec 13, 2014

Conversation

@mpapis
Copy link
Member

@mpapis mpapis commented Nov 23, 2014

#492 with whitelist support

@mpapis mpapis force-pushed the feature/dotfiles_gh_492 branch from 4f0595b to 96628bf Nov 23, 2014
#
# @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)
Copy link
Member

@ddfreyne ddfreyne Nov 29, 2014

Choose a reason for hiding this comment

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

The addition of the new extra_files param makes it not backwards compatible. Could this be converted into a params = {} defaulting to []?

Loading

Copy link
Member Author

@mpapis mpapis Nov 29, 2014

Choose a reason for hiding this comment

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

https://github.com/nanoc/nanoc/blob/feature/dotfiles_gh_492/lib/nanoc/extra/filesystem_tools.rb#L7 :

# @api private

wouldn't that mean we do not have to care about backwards compatibility? ... not that I can't do that change, just curious.

Loading

Copy link
Member

@ddfreyne ddfreyne Nov 29, 2014

Choose a reason for hiding this comment

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

You’re right. I didn’t spot the @api private. So, never mind :)

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 29, 2014

More in-depth review coming soon!

Loading

@ddfreyne ddfreyne added this to the 3.7.5 milestone Nov 29, 2014
@ddfreyne ddfreyne added this to the 3.7.5 milestone Nov 29, 2014
when String
patterns << "#{dir_name}/#{extra_files}"
when Array
patterns += extra_files.map { |extra_file| "#{dir_name}/#{extra_file}" }
Copy link
Member

@ddfreyne ddfreyne Nov 30, 2014

Choose a reason for hiding this comment

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

For consistency, I’d rather have this be

extra_files.each { |extra_file| patterns << "#{dir_name}/#{extra_file}" }

Loading

Copy link
Member Author

@mpapis mpapis Nov 30, 2014

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member

@ddfreyne ddfreyne Nov 30, 2014

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@mpapis mpapis Nov 30, 2014

Choose a reason for hiding this comment

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

I think it might be slower ... but I will do the fix in the evening

Loading

Copy link
Member

@ddfreyne ddfreyne Nov 30, 2014

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@mpapis mpapis Nov 30, 2014

Choose a reason for hiding this comment

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

that's surprising, will have to report a bug for ruby

Loading

Copy link
Member

@ddfreyne ddfreyne Nov 30, 2014

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@mpapis mpapis Dec 1, 2014

Choose a reason for hiding this comment

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

well I was expecting += to behave as concat => jruby/jruby#2256

Loading

Copy link
Member

@ddfreyne ddfreyne Dec 6, 2014

Choose a reason for hiding this comment

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

I prefer #concat:

patterns.concat(extra_files.map { |extra_file| "#{dir_name}/#{extra_file}" })

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 30, 2014

You’ll probably have to rebase against release-3.7.x because of the style changes, sorry!

Loading

@mpapis mpapis force-pushed the feature/dotfiles_gh_492 branch from 96628bf to 0859e20 Dec 1, 2014
@coveralls
Copy link

@coveralls coveralls commented Dec 1, 2014

Coverage Status

Coverage increased (+0.02%) when pulling 7e8d183 on feature/dotfiles_gh_492 into 07af041 on release-3.7.x.

Loading

@mpapis
Copy link
Member Author

@mpapis mpapis commented Dec 1, 2014

I think the remaining bug will be fixed with #502

Loading

@mpapis
Copy link
Member Author

@mpapis mpapis commented Dec 1, 2014

by bug I meant test failures

Loading

@mpapis mpapis force-pushed the feature/dotfiles_gh_492 branch from 7e8d183 to 1782d70 Dec 6, 2014
when String
patterns << "#{dir_name}/#{extra_files}"
when Array
patterns.concat(extra_files.map { |extra_file| "#{dir_name}/#{extra_file}" })
Copy link
Member Author

@mpapis mpapis Dec 6, 2014

Choose a reason for hiding this comment

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

now, is << and concat ok, or should I change << to concat([...])?

Loading

Copy link
Member

@ddfreyne ddfreyne Dec 13, 2014

Choose a reason for hiding this comment

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

Yup, that’s OK. It doesn’t make sense to use #concat when appending only a single element.

Loading

@mpapis
Copy link
Member Author

@mpapis mpapis commented Dec 6, 2014

and the tests pass after rebasing on top of merged #502

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Dec 13, 2014

👍

Loading

ddfreyne added a commit that referenced this issue Dec 13, 2014
@ddfreyne ddfreyne merged commit 82f9a88 into release-3.7.x Dec 13, 2014
1 check passed
Loading
@ddfreyne ddfreyne deleted the feature/dotfiles_gh_492 branch Dec 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants