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

Add rationale for Rails/FilePath rule #128

Closed
wants to merge 1 commit into from
Closed

Add rationale for Rails/FilePath rule #128

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 12, 2019

This was previously included in the documentation before rubocop-rails was split off from Rubocop: https://web.archive.org/web/20180307163045/https://rubocop.readthedocs.io/en/latest/cops_rails/#railsfilepath

This was previously included in the documentation before rubocop-rails was split off from Rubocop: https://web.archive.org/web/20180307163045/https://rubocop.readthedocs.io/en/latest/cops_rails/#railsfilepath
@koic
Copy link
Member

koic commented Sep 12, 2019

It has been updated by the PR rubocop/rubocop#5835.

Rails.root is a Pathname instance. Pathname resolves differences in OS path expressions.

Rails.root.class #=> Pathname

It seems that / separator doesn't really matter.
https://github.com/rubocop-hq/rubocop/issues/5823

Therefore, I'm considering changing the / separator style (EnforcedStyle: slashes) to the default.

@koic koic closed this Sep 12, 2019
@ghost ghost deleted the patch-1 branch September 12, 2019 15:55
@ghost
Copy link
Author

ghost commented Sep 12, 2019

Thanks for your response :)

koic added a commit to koic/rubocop-rails that referenced this pull request Oct 13, 2019
This PR changes default of `EnforcedStyle` from `arguments` to `slashes`
for `Rails/FilePath`.

The following is why `slashes` by default.

-`Rails.root` is an instance of `Pathname`, so the separator (`/`) works
 for example on the Windows platform.
- Since it can be expressed by a single string, the number of string
 instances can be reduced.
- Clarified because the argument is fixed to one.

`EnforcedStyle`'s default was `arguments` because it was mainly due to
Windows platform considerations, but it seems to have been a mistake.

rubocop#128 (comment)
koic added a commit to koic/rubocop-rails that referenced this pull request Oct 13, 2019
This PR changes default of `EnforcedStyle` from `arguments` to `slashes`
for `Rails/FilePath`.

The following is why `slashes` by default.

- `Rails.root` is an instance of `Pathname`, so the separator (`/`) works
 for example on the Windows platform.
- Since it can be expressed by a single string, the number of string
 instances can be reduced.
- Clarified because the argument is fixed to one.

`EnforcedStyle`'s default was `arguments` because it was mainly due to
Windows platform considerations, but it seems to have been a mistake.

rubocop#128 (comment)
koic added a commit to koic/rubocop-rails that referenced this pull request Nov 21, 2019
This PR changes default of `EnforcedStyle` from `arguments` to `slashes`
for `Rails/FilePath`.

The following is why `slashes` by default.

- `Rails.root` is an instance of `Pathname`, so the separator (`/`) works
 for example on the Windows platform.
- Since it can be expressed by a single string, the number of string
 instances can be reduced.
- Clarified because the argument is fixed to one.

`EnforcedStyle`'s default was `arguments` because it was mainly due to
Windows platform considerations, but it seems to have been a mistake.

rubocop#128 (comment)
@Lewiscowles1986
Copy link

regardless of if it works for you this is a significant departure, likely depending on ruby internals, or OS quirks. As a multi-OS user, I can remember windows not working with a /

This is resulting in a number of pointless changes, which at best reads like a litany of errors, from what is often considered to be best practice across multiple other languages.

File.join while superficially similar to just adding a '/', presents clarity for the reader as they can see each level in a hierarchy of folders. It fixes a case of accidentally entering // which when changing values can happen.

consider

given somevar='/www'

when File.join('/', '/var/', somevar)

then /var/www

vs

when ['/', '/var/', somevar].join('/')

then ///var///www

vs

when "/var/#{somevar}/"

then /var//www/

It's basing decisions on quirks (which may well work), but are not the the standard expected by a potential OS.

pgwillia added a commit to ualbertalib/jupiter that referenced this pull request Jan 3, 2020
In the gem bump for rubocop-rails to 2.4.1 the default EnforcementStyle
changed from arguments to slashes.  That means that
`Rails.root.join('config', 'controlled_vocabularies', '*.yml')` becomes
`Rails.root.join('config/controlled_vocabularies/*.yml')`.  It was
[previously thought](rubocop/rubocop-rails#128 (comment))
that the first way was OS agnostic but it turns out that Rails.root is a
Pathname instance which accounts for this.  Now the slash is prefered for
fewer strings/arguments.
pgwillia added a commit to ualbertalib/jupiter that referenced this pull request Jan 3, 2020
In the gem bump for rubocop-rails to 2.4.1 the default EnforcementStyle
changed from arguments to slashes.  That means that
`Rails.root.join('config', 'controlled_vocabularies', '*.yml')` becomes
`Rails.root.join('config/controlled_vocabularies/*.yml')`.  It was
[previously thought](rubocop/rubocop-rails#128 (comment))
that the first way was OS agnostic but it turns out that Rails.root is a
Pathname instance which accounts for this.  Now the slash is prefered for
fewer strings/arguments.
lagoan pushed a commit to ualbertalib/jupiter that referenced this pull request Jan 3, 2020
* Bump rubocop-rails from 2.3.2 to 2.4.1

Bumps [rubocop-rails](https://github.com/rubocop-hq/rubocop-rails) from 2.3.2 to 2.4.1.
- [Release notes](https://github.com/rubocop-hq/rubocop-rails/releases)
- [Changelog](https://github.com/rubocop-hq/rubocop-rails/blob/master/CHANGELOG.md)
- [Commits](rubocop/rubocop-rails@v2.3.2...v2.4.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

* Rails/FilePath default changed to slashes

In the gem bump for rubocop-rails to 2.4.1 the default EnforcementStyle
changed from arguments to slashes.  That means that
`Rails.root.join('config', 'controlled_vocabularies', '*.yml')` becomes
`Rails.root.join('config/controlled_vocabularies/*.yml')`.  It was
[previously thought](rubocop/rubocop-rails#128 (comment))
that the first way was OS agnostic but it turns out that Rails.root is a
Pathname instance which accounts for this.  Now the slash is prefered for
fewer strings/arguments.

Co-authored-by: pgwillia <tricia.g.jenkins@gmail.com>
@sunny
Copy link
Contributor

sunny commented Aug 3, 2021

As a multi-OS user, I can remember windows not working with a /

I thought so too and so did many people in the Ruby community, but it turns out that it is not the case.

File.join while superficially similar to just adding a '/', presents clarity for the reader as they can see each level in a hierarchy of folders. It fixes a case of accidentally entering // which when changing values can happen.

You are right, File.join is great for concatenating paths as it helps handle extra slashes in a neat way. We should definitely keep using it.

However, when your paths are not variables but strings like so:

File.join('/', '/var/', somevar)
File.join('/usr', 'var', 'logs', 'myapp', 'daily', somevar)

It doesn’t make much practical sense here to split this in multiple Strings. It can be simply written:

File.join('/var', somevar)
File.join('/usr/var/logs/myapp/daily', somevar)

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