Feature #4970
FileUtils refactored
| 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".
Related issues
Associated revisions
History
#1
Updated by Thomas Sawyer about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 3 years ago
- Status changed from Open to Closed
- % Done changed from 0 to 100