Skip to content

Conversation

@glacials
Copy link
Contributor

This allows $LOAD_PATH to contain Pathname objects in addition to strings. Previously Pathnames would break any attempt to precompile assets because of an undefined match method.

@kou
Copy link
Member

kou commented May 23, 2014

Your suggestion is reasonable but we should use #to_path instead of #to_s for the case.
Could you fix it?

@glacials
Copy link
Contributor Author

We can't call to_path on strings, so we'd need to have a condition to check for Pathnames and call to_path on them while letting strings through normally.

This would be fine if to_path were more appropriate than to_s, but they're literally the exact same method, so there would be no advantage to using to_path here (any gained semantic clarity would be fogged up by the additional type check imo).

That said, I can still change this if you'd like me to.

@kou
Copy link
Member

kou commented May 24, 2014

In your use case, I agree with using #to_path is no advantage. Because you just need to care about String and Pathname.

But gettext gem is a library that is used by more users. They may put a path object that responds to #to_path to $LOAD_PATH. For example, we can put Dir object to $LOAD_PATH. Because Dir#to_path exists.

Because of the above reason, I prefer to use #to_path rather than #to_s.

It is better that using path.respond_to?(:to_path) rather than path.is_a?(Pathname) for checking whether we should call #to_path or not.

If you agree with my opinion, could you use #to_path?

@glacials
Copy link
Contributor Author

That's a really good point! I hadn't thought of that. Here's the change.

kou added a commit that referenced this pull request May 28, 2014
Support Pathnames in $LOAD_PATH

Patch by Ben Carlsson. Thanks!!!
@kou kou merged commit 18af996 into ruby-gettext:master May 28, 2014
@kou
Copy link
Member

kou commented May 28, 2014

Thanks!!!
I've merged your pull request. :-)

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.

2 participants