Feature #4970: FileUtils refactored

• Status: Closed
• Priority: Normal
• Assignee: -
• Target version: 2.0.0

[trans (Thomas Sawyer)]

I've been working with FileUtils a good bit, and concluded it could use some refactoring to make the code clearer and easier to work with. Here is the pull request:

https://github.com/ruby/ruby/pull/30

Essentially, I have removed the method definition loops that occur at the end of the script and replaced them with a simple call (define_command) made for each command as it is defined. This allowed me to use extend self all the way through, rather than having to use module_function in FileUtils and extend self in the Verbose, NoWrite and DryRun "submodules".

[trans (Thomas Sawyer)]

Is current trunk destined to be 2.0? If so, can this get a review and merge if ok?

[matz (Yukihiro Matsumoto)]

Hi,

In message "Re: [ruby-trunk - Feature #4970] FileUtils refactored"
on Wed, 28 Dec 2011 11:54:52 +0900, Thomas Sawyer [e-mail] writes:

|Is current trunk destined to be 2.0? If so, can this get a review and merge if ok?

Current trunk is to be 2.0. Is anyone willing to review the code?

                        matz.

[trans (Thomas Sawyer)]

Aaron Patterson looked at it, his only remarks were that I forgot to remove a spurious comment and that I changed the indention on private. Since, he said nothing about the implementation itself, I am assuming it looked okay to him.

I would remove the unnecessary comment myself, but I seem to have deleted the repo I was working on, and I am not sure there is a way to get it back such that I can update the same pull request. It would just be easier to merge then remove the comment, and if deemed necessary, rebase to a single commit.

On Thu, Feb 16, 2012 at 02:26:10AM +0900, Thomas Sawyer wrote:

Issue #4970 has been updated by Thomas Sawyer.

Aaron Patterson looked at it, his only remarks were that I forgot to remove a spurious comment and that I changed the indention on private. Since, he said nothing about the implementation itself, I am assuming it looked okay to him.

Ya, I think it's basically fine. I have a few more questions that I'll
add to the diff. Sorry it's taking me so long to respond on this. :(

I would remove the unnecessary comment myself, but I seem to have deleted the repo I was working on, and I am not sure there is a way to get it back such that I can update the same pull request. It would just be easier to merge then remove the comment, and if deemed necessary, rebase to a single commit.

I don't think it matters too much. Once we have the final patch
assembled, I can just apply to trunk without the pull request.

--
Aaron Patterson
http://tenderlovemaking.com/

[mame (Yusuke Endoh)]

Hello,

2012/2/16 Aaron Patterson [e-mail]:

Ya, I think it's basically fine.

Agreed. Also looks good to me. Thanks.

--
Yusuke Endoh [e-mail]

[h.shirosaki (Hiroshi Shirosaki)]

r34669 (trunk): * lib/fileutils.rb: refactored FileUtil methods
seems to cause test errors with mingw32.

TestDir_M17N#test_filename_as_bytes_extutf8 = 0.12 s = E
TestDir_M17N#test_filename_extutf8_inteucjp_unrepresentable = 0.18 s = E

This patch reverts a line to the original code and fixes errors.

diff --git a/lib/fileutils.rb b/lib/fileutils.rb
index 8d1009b..46dfffb 100644
--- a/lib/fileutils.rb
+++ b/lib/fileutils.rb
@@ -1342,7 +1342,7 @@ private

 def entries
   opts = {}

• opts[:encoding] = "UTF-8" if /mswin|mignw/ =~ RUBY_PLATFORM
• opts[:encoding] = ::Encoding::UTF_8 if fu_windows? Dir.entries(path(), opts)\ .reject {|n| n == '.' or n == '..' }\ .map {|n| Entry_.new(prefix(), join(rel(), n.untaint)) }

[trans (Thomas Sawyer)]

Looks like someone had simply misspelled "mignw". I am guessing that's actually the older code, and fu_windows? is the newer. Is that right?

The refactoring I did did not touch that line, so I am guessing it was changed between the time I wrote the refactor and now --which could be since 1) it was a number of months ago and 2) I lost my original branch and had to reconstruct the the whole file and resubmit.

[nobu (Nobuyoshi Nakada)]

(12/02/18 14:56), Thomas Sawyer wrote:

Looks like someone had simply misspelled "mignw". I am guessing that's actually the older code, and fu_windows? is the newer. Is that right?

Right. I fixed it at r34146, in the last December.

• Status changed from Open to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r34706.
Thomas, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• lib/fileutils.rb: revert a line modified accidentally at r34669. This fixes mingw test errors in TestDir_M17N. [Feature #4970]