Feature #4970
closedFileUtils refactored
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
Updated by trans (Thomas Sawyer) almost 13 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) almost 13 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 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.
Updated by trans (Thomas Sawyer) almost 13 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 almost 13 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) almost 13 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
Updated by h.shirosaki (Hiroshi Shirosaki) almost 13 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) almost 13 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) almost 13 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.
Updated by Anonymous almost 13 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]