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 explicit conversion Pathname -> String #8113

Merged
merged 1 commit into from
Dec 15, 2014
Merged

Add explicit conversion Pathname -> String #8113

merged 1 commit into from
Dec 15, 2014

Conversation

radeksimko
Copy link
Contributor

This is related to #8022 and some of our rspec tests (after adding fakefs bootstrap) will fail while calling this code because it relies on automatic conversion from object to String (via to_str).

I first thought I'm gonna send a PR to fakefs to implement the to_str method, but after doing a quick search I rather decided to do explicit conversion here and here's why:
https://www.ruby-forum.com/topic/4405700

I'm actually surprised that the tests are still green on Ruby 2.0.0+.

I was also thinking that we could add it elsewhere where we work with Pathname as String, but this is currently the one place which is heavily covered with tests and which bothers me most + I want to see your feedback on suggested approach first.

Feel free to run the tests with fakefs to see the failures:
https://github.com/radeksimko/homebrew-cask/commit/fe1095b5c81992aaa1e6c39a6afb0931d1191b30

It is only problem when ignoring the shutup helper, so you will need to use the environment variable from here:
#8101

@rolandwalker
Copy link
Contributor

This change does no harm, as it makes explicit what system does implicitly. At some point, before Ruby gets down to invoking execve, everything must become a C array of char.

However, as system always invokes this to_s implicitly, fakefs should be patched as well, if possible. From a quick glance, I wasn't sure whether they intend to cover system at all, or whether they just mock native methods in FileUtils and Pathname.

Fakefs looks like a step in the right direction for us. Our core functionality is changing state on the disk, but we don't always test whether we have changed state on a disk! A relevant recent bug was 2e71e51 – we were really only testing the mocking.

The reason our existing Pathname code works fine (and is conceptually just fine) is the difference between the to_s and to_str methods. We depend on to_s, which is OK because any Pathname (any Object) always has a (perhaps lossy) string representation. We don't depend on to_str, because a Pathname instance doesn't respond to all String methods in a String-like way.

(Though there are plenty of places the distinction is muddied, especially in the test suite. We inherited Pathname#/ from Homebrew, which I find confusing, and would like to remove after #8089).

rolandwalker added a commit that referenced this pull request Dec 15, 2014
Add explicit conversion Pathname -> String
@rolandwalker rolandwalker merged commit 8c92637 into Homebrew:master Dec 15, 2014
@radeksimko radeksimko deleted the object-string-conversion branch December 15, 2014 14:52
@rolandwalker
Copy link
Contributor

@radeksimko you were right. We have tons of code that depends on this alias from Homebrew .

This is a case where, because this is the only Ruby project I have worked on, the stuff we implicitly pulled in from Homebrew has warped my understanding of how Ruby actually works. Contrary to what I wrote above, system attempts to_str.

While it's clear which bugs they were addressing with the change to the Pathname class, I'd argue that the behavior of system here is poor. The Pathname arguments are in isolation, there is no other possible interpretation, and paths are something frequently passed to system. So, system ought to invoke to_path.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants