Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 #337

gipcompany · 2015-12-26T19:21:11Z

dblock · 2015-12-28T15:29:18Z

This code is good.

I want to leave this open for others to comment, in theory the caller can just do to_s on the Pathname and we won't need this merged. I don't see many gems supporting Pathname directly, I am not convinced we should.

gipcompany · 2015-12-28T20:35:15Z

@dblock thank you very much for taking your time to review the code. I hope to see others' feedbacks or opinions.

gregory · 2016-01-14T14:46:59Z

@gipcompany why do you feel that we need to be able to handle Pathname objects instead of a plain string?
Can't you just do Mash.load(Pathname('your_path').to_s) ?

michaelherold · 2016-02-01T00:14:24Z

The only thing I'm not a fan of is the switching based on class. An alternative to this would be to just always call #to_s on the parameter, a la:

@file_path = file_path.to_s

This doesn't break the current implementation and allows you to use Pathname or any other type of file pointer that responds to #to_s. Since we're expecting a string here anyway, it's semantically the same but expands the usable parameter space.

dblock · 2016-02-01T15:38:55Z

The issue with things like to_s is that you start opening files by passing garbage in, and that is bound to give us some security issues somewhere in a stack of things at some point.

najamelan · 2016-07-14T14:25:20Z

To bad this didn't get merged. I subclass Mash just to get this... o well.

I also don't understand the argument about passing garbage in. If it resolves to an existing filename, then apparently it isn't garbage, and in the end the user chooses what they pass in... and as far as I'm concerned it's their responsibility not to pass in "garbage" that resolves to "/etc/shadow". In any case files here should be opened read only, so what's the security implication?

I think it makes sense that code that operates on files can at least take in Pathname instead of a string. But personally why not everything that responds to #to_str, since it's up to the user to send something sensible in. If you want to be sure that it is a path, and not garbage, it's a small fix:

path.kind_of?( Pathname ) and path = path.to_s

dblock · 2016-07-14T14:38:56Z

CHANGELOG.md

@@ -4,8 +4,10 @@
 * [#319](https://github.com/intridea/hashie/pull/319): Fix a regression from 3.4.1 where `Hashie::Extensions::DeepFind` is no longer indifference-aware - [@michaelherold](https://github.com/michaelherold).
 * [#240](https://github.com/intridea/hashie/pull/240): Fixed nesting twice with Clash keys - [@bartoszkopinski](https://github.com/bartoszkopinski).
 * [#322](https://github.com/intridea/hashie/pull/322): Fixed `reverse_merge` issue with `Mash` subclasses - [@marshall-lee](https://github.com/marshall-lee).
+* [#337](https://github.com/intridea/hashie/pull/337): Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 - [@gipcompany](https://github.com/gipcompany).


Should just say Hashie::Mash.load accepts a Pathname, remove "implemented a new feature" and "object", quote everything that's code.

dblock · 2016-07-14T14:40:37Z

@najamelan I think you're right, and I am ok with the code as it stands. @michaelherold LMK how you feel about this?

file_path.is_a?(Pathname) ? file_path.to_s : file_path

dblock · 2016-07-14T14:40:45Z

This should be rebased, build fixed.

michaelherold · 2016-07-14T14:48:08Z

LGTM. 👍

dblock · 2016-07-14T14:51:47Z

@gipcompany sorry for the delay, see above, would appreciate if you could help wrap this up

najamelan · 2016-07-14T15:43:22Z

@dblock Thanks

gipcompany · 2016-07-14T16:22:29Z

@dblock let me see. I would like to catch up with the current status. Should I resolve this PR not to be conflicted and re-push it?

dblock · 2016-07-15T12:56:22Z

Lets fix the build. ActiveSupport 5 is in the way now I think with older rubies, just add ~> 4.x into https://github.com/intridea/hashie/blob/master/Gemfile#L15 for now, that should fix it.

dblock · 2016-07-15T12:56:52Z

CHANGELOG.md

@@ -12,7 +12,7 @@ scheme are considered to be bugs.

 ### Added

-* Nothing yet.
+* [#337](https://github.com/intridea/hashie/pull/337): Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 - [@gipcompany](https://github.com/gipcompany).


This should say "Hashie::Mash.load accepts a Pathname - ...". This is too verbose otherwise :)

dblock · 2016-07-15T12:57:44Z

Try to rebase this against master. You should end up with a single commit here. Git can be fun.

…bject hashie#331 hashie#331

dblock · 2016-07-15T15:58:23Z

Merged via bbafade.

gipcompany · 2016-07-15T16:10:20Z

Wow, this being merged in public is my first time experience. Thank you everyone for each support and thank you, @dblock for your continuous support.

dblock · 2016-07-15T17:41:42Z

Also hashes in commits that contain dictionary words, like fade in bbafade are a big deal, congrats on your first open source contribution!

najamelan · 2016-07-18T01:09:30Z

I'm sorry to arrive a bit late with this, but as I'm implementing something similar, I came to the following conclusion:

Pathname, File, but also Dir all implement #to_path which gives the string path... In some sense it seems more logical to check for objects responding to #to_path and to use that instead of #to_s. It would require a check for directory? if you don't support loading all files in a directory though.

dblock · 2016-07-18T20:43:35Z

You have some good points @najamelan, feel free to PR an update!

@jrafanie

[3.4.6]: hashie/hashie@v3.4.5...v3.4.6 ### Fixed * [#368](hashie/hashie#368): Since `hashie/mash` can be required alone, require its dependencies - [@jrafanie](https://github.com/jrafanie). ## [3.4.5] - 2016-09-16 [3.4.5]: hashie/hashie@v3.4.4...v3.4.5 ### Added * [#337](hashie/hashie#337), [#331](hashie/hashie#331): `Hashie::Mash#load` accepts a `Pathname` object - [@gipcompany](https://github.com/gipcompany). ### Deprecated * [#366](hashie/hashie#366): Hashie is no longer tested on Ruby < 2 - [@dblock](https://github.com/dblock). ### Fixed * [#358](hashie/hashie#358): Fixed support for Array#dig - [@modosc](https://github.com/modosc). * [#365](hashie/hashie#365): Ensured ActiveSupport::HashWithIndifferentAccess is defined before use in #deep_locate - [@mikejarema](https://github.com/mikejarema). ### Miscellanous * [#366](hashie/hashie#366): Added Danger, PR linter - [@dblock](https://github.com/dblock). ## [3.4.4] - 2016-04-29 [3.4.4]: hashie/hashie@v3.4.3...v3.4.4 ### Added * [#349](hashie/hashie#349): Convert `Hashie::Mash#dig` arguments for Ruby 2.3.0 - [@k0kubun](https://github.com/k0kubun). ### Fixed * [#240](hashie/hashie#240): Fixed nesting twice with Clash keys - [@bartoszkopinski](https://github.com/bartoszkopinski). * [#317](hashie/hashie#317): Ensure `Hashie::Extensions::MethodQuery` methods return boolean values - [@michaelherold](https://github.com/michaelherold). * [#319](hashie/hashie#319): Fix a regression from 3.4.1 where `Hashie::Extensions::DeepFind` is no longer indifference-aware - [@michaelherold](https://github.com/michaelherold). * [#322](hashie/hashie#322): Fixed `reverse_merge` issue with `Mash` subclasses - [@marshall-lee](https://github.com/marshall-lee). * [#346](hashie/hashie#346): Fixed `merge` breaking indifferent access - [@docwhat](https://github.com/docwhat), [@michaelherold](https://github.com/michaelherold). * [#350](hashie/hashie#350): Fixed from string translations used with `IgnoreUndeclared` - [@marshall-lee](https://github.com/marshall-lee).

gipcompany mentioned this pull request Dec 26, 2015

implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 #336

Closed

gipcompany force-pushed the 331_new_feature branch from 089a0f8 to 3d3282e Compare December 27, 2015 01:19

dblock reviewed Jul 14, 2016
View reviewed changes

dblock reviewed Jul 15, 2016
View reviewed changes

gipcompany force-pushed the 331_new_feature branch from 6137e57 to 7a7d2e3 Compare July 15, 2016 15:10

Implemented a new feature that Hashie::Mash.load accepts a Pathname o…

39590fc

…bject hashie#331 hashie#331

gipcompany force-pushed the 331_new_feature branch from 7a7d2e3 to 39590fc Compare July 15, 2016 15:12

add activesupport version for test for now

23809c9

gipcompany force-pushed the 331_new_feature branch from 12b70e8 to 23809c9 Compare July 15, 2016 15:34

dblock closed this Jul 15, 2016

gipcompany deleted the 331_new_feature branch July 23, 2016 04:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 #337

Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 #337

gipcompany commented Dec 26, 2015

dblock commented Dec 28, 2015

gipcompany commented Dec 28, 2015

gregory commented Jan 14, 2016

michaelherold commented Feb 1, 2016

dblock commented Feb 1, 2016

najamelan commented Jul 14, 2016

dblock Jul 14, 2016

dblock commented Jul 14, 2016

dblock commented Jul 14, 2016 •

edited

michaelherold commented Jul 14, 2016

dblock commented Jul 14, 2016

najamelan commented Jul 14, 2016

gipcompany commented Jul 14, 2016

dblock commented Jul 15, 2016

dblock Jul 15, 2016 •

edited

dblock commented Jul 15, 2016

dblock commented Jul 15, 2016

gipcompany commented Jul 15, 2016

dblock commented Jul 15, 2016

najamelan commented Jul 18, 2016 •

edited

dblock commented Jul 18, 2016

Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 #337

Implemented a new feature that Hashie::Mash.load accepts a Pathname object #331 #337

Conversation

gipcompany commented Dec 26, 2015

dblock commented Dec 28, 2015

gipcompany commented Dec 28, 2015

gregory commented Jan 14, 2016

michaelherold commented Feb 1, 2016

dblock commented Feb 1, 2016

najamelan commented Jul 14, 2016

dblock Jul 14, 2016

Choose a reason for hiding this comment

dblock commented Jul 14, 2016

dblock commented Jul 14, 2016 • edited

michaelherold commented Jul 14, 2016

dblock commented Jul 14, 2016

najamelan commented Jul 14, 2016

gipcompany commented Jul 14, 2016

dblock commented Jul 15, 2016

dblock Jul 15, 2016 • edited

Choose a reason for hiding this comment

dblock commented Jul 15, 2016

dblock commented Jul 15, 2016

gipcompany commented Jul 15, 2016

dblock commented Jul 15, 2016

najamelan commented Jul 18, 2016 • edited

dblock commented Jul 18, 2016

dblock commented Jul 14, 2016 •

edited

dblock Jul 15, 2016 •

edited

najamelan commented Jul 18, 2016 •

edited