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

Prefer using slashes in Rails/FilePath cop #195

Closed
sunny opened this issue Apr 25, 2018 · 5 comments
Closed

Prefer using slashes in Rails/FilePath cop #195

sunny opened this issue Apr 25, 2018 · 5 comments

Comments

@sunny
Copy link
Contributor

sunny commented Apr 25, 2018

Under Windows, Rails.root.join('app', 'models', 'goober') is equal to Rails.root.join('app/models/goober'). They both return a string with forward slashes.

I'm in favor of always using the simple slashes and deprecating the previous join. The current cop suggests this is

to avoid bugs on operating system that don't use '/' as the path separator

but there is no evidence of such bugs.

More discussion of the issue here: https://twitter.com/tenderlove/status/842064491936280576

c6-l0cfwyaa6rvc


Expected behavior

Rails/FilePath should treat as:

# bad
Rails.root.join('app', 'models', 'goober')

# good
Rails.root.join('app/models/goober')

Actual behavior

Today it treats the following as:

# good
Rails.root.join('app', 'models', 'goober')

# bad
Rails.root.join('app/models/goober')
@roberts1000
Copy link

Good find. FWIW, we disable this cop at our shop and use the "#{Rails.root}/app/models/goober" format.

@Drenmi
Copy link
Contributor

Drenmi commented Apr 25, 2018

At the very least the style should be configurable.

@juanxcerv
Copy link

When trying to integrate this change we discovered that there is a difference in how absolute paths are resolved with each approach.

For example:

path = File.expand_path('./app/models/user.rb')
# => "/Users/juancervantes/Desktop/synchroform/app/models/user.rb"
Rails.root.join('app', 'models', path)
# => #<Pathname:/Users/juancervantes/Desktop/synchroform/app/models/user.rb>
Rails.root.join("app/models/#{path}")
# => #<Pathname:/Users/juancervantes/Desktop/synchroform/app/models//Users/juancervantes/Desktop/synchroform/app/models/user.rb>

We decided to stick with the separate arguments for the sake of consistency.

@koic koic transferred this issue from rubocop/rubocop Jan 30, 2020
camdenmoors added a commit to camdenmoors/ctf-scoreboard that referenced this issue Jun 11, 2020
Rubocop doesn't like it when I use the form: ``Rails.root.join('some', 'path', 'here')``` and wants me to use the more OS dependent ```Rails.root.join('some/path/here')```. See rubocop/rubocop-rails#195 (comment)
camdenmoors added a commit to camdenmoors/ctf-scoreboard that referenced this issue Jun 11, 2020
Rubocop doesn't like it when I use the form: ``Rails.root.join('some', 'path', 'here')``` and wants me to use the more OS dependent ```Rails.root.join('some/path/here')```. See rubocop/rubocop-rails#195 (comment)
@sunny
Copy link
Contributor Author

sunny commented Aug 3, 2021

@juanxcerv:

When trying to integrate this change we discovered that there is a difference in how absolute paths are resolved with each approach.

When building paths, instead of using String interpolation Rails.root.join("app/models/#{path}") you should still use join to do the concatenation: Rails.root.join("app/models", path).

Although beware that in your example you are joining a path that starts with /. This is actually dismissing whatever was used beforehand. This can be quite unexpected: in your example Rails.root.join('app', 'models', path) ignores anything before path 🙀

path = File.expand_path('./app/models/user.rb')
# => "/Users/juancervantes/Desktop/synchroform/app/models/user.rb"
Rails.root.join('app', 'models', path)
# => #<Pathname:/Users/juancervantes/Desktop/synchroform/app/models/user.rb>
Rails.root.join('app/models', path)
# => #<Pathname:/Users/juancervantes/Desktop/synchroform/app/models/user.rb>
Rails.root.join('whatever', 'since', 'this', 'is', 'ignored', path)
# => #<Pathname:/Users/juancervantes/Desktop/synchroform/app/models/user.rb>
Pathname.new(path)
# => #<Pathname:/Users/juancervantes/Desktop/synchroform/app/models/user.rb>

So do not concatenate expanded paths, simply do:

path = "app/models/user.rb"
Rails.root.join('app/models', path)

@philliplongman
Copy link

I feel like this should be revisited in light of @sunny's comment. It sounds like what this cop should be doing is guarding against string interpolation.

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

No branches or pull requests

5 participants