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

Support Windows paths #698

Closed
wants to merge 8 commits into from
Closed

Support Windows paths #698

wants to merge 8 commits into from

Conversation

rifraf
Copy link
Contributor

@rifraf rifraf commented Dec 27, 2013

This change allows me to type rubocop c:\d\e in Windows instead of c:/d/e.
In particular, if I use sublime_rubocop, this allows me to check projects and folders

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.41%) when pulling 6984e81 on rifraf:master into 0c09856 on bbatsov:master.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 30, 2013

The supplied patch is not working correctly (see the failing tests).

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 31, 2013

Btw, @jonas054 @yujinakayama any thoughts on this? I'm wondering what's the best way to handle this, but I haven't dealt with Windows paths in ages.

@jonas054
Copy link
Collaborator

I'm not sure why we have a problem in the first place. Is it because we're assuming that the file separator is / in our code? Either way, if this patch, with an added if File::ALT_SEPARATOR, solves the problem on Windows, then I think all is well. I don't have a Windows environment set up to test anything, though.

@rifraf
Copy link
Contributor Author

rifraf commented Dec 31, 2013

Yes - it is simply that (as stated in the readme) paths are / separated when entered on the command-line. A Windows user would expect to be using .
It can be argued that it is correct according to the documentation, but it took me a while to realise it. Also sublime_rubocop gets it wrong - it uses \ separators.
Thanks for the attention

@yujinakayama
Copy link
Collaborator

I have never dealt with Windows paths too.

My initial concerns were:

  1. Is this a common way to handle Windows paths in CLI tools written in Ruby?
  2. Doesn't this have a harmful effect on non-Windows platforms?

Then I've investigated a bit:

  1. RSpec also uses this way. fixes #396 rspec/rspec-core#605
  2. Ruby always uses File::SEPARATOR (/) as an internal path separator regardless of platform. File::ALT_SEPARATOR would have a value if the platform's file system uses another path separator (ref in ja). On Un*x, it's nil. So I think it won't harm with if File::ALT_SEPARATOR condition.

Thus, I think it's ok if we add if File::ALT_SEPARATOR.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 31, 2013

@yujinakayama Excellent research.

@rifraf Please, update your PR according to @yujinakayama's comments and update the changelog as well.

File::ALT_SEPARATOR can be nil on non-Windows...
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 6028dd6 on rifraf:master into 0c09856 on bbatsov:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling a9fc439 on rifraf:master into ca2a23c on bbatsov:master.

@@ -10,6 +10,7 @@

### Bugs fixed

* [#698](https://github.com/bbatsov/rubocop/pull/698): Support Windows paths on command-line. ([@rifraf][])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add your nickname to the list at the bottom of the file as well and squash the commits together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - I'm at the limits of my knowledge here. I wasn't really expecting all these magic tests to run (and complain) while I thought I was working quietly on a fork where no-one was watching...

I don't know how to squash the commits - can you help?
Also am I already updating the original PR or do I ship a new one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. I've squashed and merged your changes manually.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 963262c on rifraf:master into ca2a23c on bbatsov:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling aaccba8 on rifraf:master into ca2a23c on bbatsov:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling 5b19885 on rifraf:master into ca2a23c on bbatsov:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling dfd6504 on rifraf:master into ca2a23c on bbatsov:master.

@bbatsov bbatsov closed this Dec 31, 2013
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.

5 participants