Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor File.expand_path usage to remove additional File.join #12752

Merged
merged 1 commit into from
Nov 5, 2013
Merged

Refactor File.expand_path usage to remove additional File.join #12752

merged 1 commit into from
Nov 5, 2013

Conversation

notalex
Copy link
Contributor

@notalex notalex commented Nov 4, 2013

File.join might not be required when File.expand_path is being used, unless we want to make it work on windows. I feel that removing the extra File.join part simplifies the code. If there is actually an advantage in using File.join with expand_path, it would be good to know.

@rafaelfranca
Copy link
Member

We want to make it work on windows. So I guess we can't change right?

@notalex
Copy link
Contributor Author

notalex commented Nov 5, 2013

@rafaelfranca, I tried it out on a windows machine inside VirtualBox. The following code works on ruby2.0.0-p247, ruby1.9.3-p448 & ruby1.8.7-374.

file_path = 'actionview/test/actionpack/abstract/helper_test.rb'
File.expand_path('../../../fixtures/helpers_missing', file_path) # => 'actionview/test/fixtures/helpers_missing'

File.expand_path('~/.railsrc')   # => 'C:/Documents and Settings/notalex/.railsrc'

Since Ruby internally deals with Windows path in the Unix way, I guess this patch would work on windows as well.

@notalex
Copy link
Contributor Author

notalex commented Nov 5, 2013

I am adding a fix for another line, which I was unsure of before.

@senny
Copy link
Member

senny commented Nov 5, 2013

@rafaelfranca even the documentation suggests the use of / inside the String passed to expand_path. This should be safe to use even for windows: http://www.ruby-doc.org/core-2.0.0/File.html#method-c-expand_path

senny added a commit that referenced this pull request Nov 5, 2013
Refactor File.expand_path usage to remove additional File.join
@senny senny merged commit 7d17b1d into rails:master Nov 5, 2013
@senny
Copy link
Member

senny commented Nov 5, 2013

@notalex thank you for your contribution 💛

@notalex notalex deleted the expand_path_refactoring branch November 5, 2013 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants