Project

General

Profile

Actions

Feature #4970

closed

FileUtils refactored

Added by trans (Thomas Sawyer) almost 11 years ago. Updated almost 10 years ago.

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

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


Files

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

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #7958: include FileUtils::Verbose gives NoMethodError when installing files with a different modeCloseddrbrain (Eric Hodel)02/26/2013Actions

Updated by trans (Thomas Sawyer) over 10 years ago

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

Updated by matz (Yukihiro Matsumoto) over 10 years ago

Hi,

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

Updated by trans (Thomas Sawyer) over 10 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.

Updated by Anonymous over 10 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/

Updated by mame (Yusuke Endoh) over 10 years ago

Hello,

2012/2/16 Aaron Patterson :

Ya, I think it's basically fine.

Agreed. Also looks good to me. Thanks.

--
Yusuke Endoh

Updated by h.shirosaki (Hiroshi Shirosaki) over 10 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)) }
    

Updated by trans (Thomas Sawyer) over 10 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.

Updated by nobu (Nobuyoshi Nakada) over 10 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.

Actions #9

Updated by Anonymous over 10 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.
    [ruby-core:42728] [Feature #4970]
Actions

Also available in: Atom PDF