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

Developers often use Pathname objects for paths #274

Closed
wants to merge 1 commit into from

Conversation

krainboltgreene
Copy link

Strings are naive path names, and in Ruby we have an object specifically for path names called Pathname.

@iainbeeston
Copy link
Contributor

I think really we should be moving away from explicit type checking and towards duck typing.

Looking at how OpenURI handles this problem, it checks for the open() method (which is supported by Pathname and IO objects) and failing that it checks for to_str() (which is supported by strings and Pathname in ruby prior to 1.9).

This method is a bit of a mess, it should probably be checking for supported methods (like OpenURI does) rather than using rescue instead of conditionals and rescue statements for flow control.

@RST-J
Copy link
Contributor

RST-J commented Jan 7, 2016

I agree with @iainbeeston, I think it's the most flexible way and having rescue for control flow is really a code smell.

@iainbeeston
Copy link
Contributor

I'm going to close this for now. Feel free to open another PR

@iainbeeston iainbeeston closed this Jan 8, 2016
@krainboltgreene
Copy link
Author

This is rather disappointing.
On Jan 8, 2016 2:50 AM, "Iain Beeston" notifications@github.com wrote:

I'm going to close this for now. Feel free to open another PR


Reply to this email directly or view it on GitHub
#274 (comment)
.

@iainbeeston
Copy link
Contributor

If you have time to make a new pull request that uses duck-typing rather than explicit type checking and we'll be more than happy to look at it

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.

3 participants