Feature #4970

FileUtils refactored

Added by Thomas Sawyer almost 4 years ago. Updated almost 3 years ago.

[ruby-core:37761]
Status:Closed
Priority:Normal
Assignee:-

Description

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".

noname (500 Bytes) Anonymous, 02/16/2012 03:23 AM


Related issues

Related to Ruby trunk - Bug #7958: include FileUtils::Verbose gives NoMethodError when installing files with a different mode Closed 02/26/2013

Associated revisions

Revision 34706
Added by shirosaki over 3 years ago

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

Revision 34706
Added by shirosaki over 3 years ago

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

History

#1 Updated by Thomas Sawyer over 3 years ago

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

#2 Updated by Yukihiro Matsumoto over 3 years ago

Hi,

In message "Re: [ruby-trunk - Feature #4970] FileUtils refactored"
on Wed, 28 Dec 2011 11:54:52 +0900, Thomas Sawyer transfire@gmail.com 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.

#3 Updated by Thomas Sawyer over 3 years ago

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.

#4 Updated by Anonymous over 3 years ago

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/

#5 Updated by Yusuke Endoh over 3 years ago

Hello,

2012/2/16 Aaron Patterson tenderlove@ruby-lang.org:

Ya, I think it's basically fine.

Agreed. Also looks good to me. Thanks.

--
Yusuke Endoh mame@tsg.ne.jp

#6 Updated by Hiroshi Shirosaki over 3 years ago

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)) }

#7 Updated by Thomas Sawyer over 3 years ago

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.

#8 Updated by Nobuyoshi Nakada over 3 years ago

(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.

#9 Updated by Anonymous over 3 years ago

  • 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]

Also available in: Atom PDF