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

Feature request: Tree.create_from_hash() #13

Closed
markthomas opened this issue Dec 1, 2012 · 8 comments
Closed

Feature request: Tree.create_from_hash() #13

markthomas opened this issue Dec 1, 2012 · 8 comments
Labels

Comments

@markthomas
Copy link

@markthomas markthomas commented Dec 1, 2012

I think RubyTree should have convenience methods to convert built-in hierarchical structures to trees. I propose Tree.create_from_hash(hash) as a class method. After all, you have JSON input/output, why not built-in data structures. I'd like for it to work with arbitrary nested hashes.

A few questions:
Is it OK as a class method? I think it makes sense.
Naming: do you prefer Tree.create_from_hash() or Tree.from_hash()?
Would you want it directly in the Tree class or split out into utility class like JSON is?

@evolve75
Copy link
Owner

@evolve75 evolve75 commented Dec 2, 2012

Sure Mark,

It is a good idea. It needs to be a class method in a utility mixin (similar to the JSON implementation).

It should probably be named as from_hash()

Can you fork the master on Github, and then send me the merge request with your changes?

Best Regards,

Anupam

On Dec 1, 2012, at 6:41 PM, Mark Thomas notifications@github.com wrote:

I think RubyTree should have convenience methods to convert built-in hierarchical structures to trees. I propose Tree.create_from_hash(hash) as a class method. After all, you have JSON input/output, why not built-in data structures. I'd like for it to work with arbitrary nested hashes.

A few questions:
Is it OK as a class method? I think it makes sense.
Naming: do you prefer Tree.create_from_hash() or Tree.from_hash()?
Would you want it directly in the Tree class or split out into utility class like JSON is?


Reply to this email directly or view it on GitHub.

@markthomas
Copy link
Author

@markthomas markthomas commented Dec 6, 2012

In my fork I have implemented a from_hash() class method as well as an instance method by the same name. However, they necessarily work differently, because the class method needs to create the root node, and there can only be one, so there would be a restriction that the hash must have exactly one element at the top level. Whereas the instance method simply adds the hash as child(ren). Having two methods with the same name that behave slightly differently would be confusing.

So my latest thinking is to keep only the instance method. To create a new tree, one would have to create a root TreeNode first, then import the hash, e.g. mytree = Tree::TreeNode.new("root").from_hash(myhash). Not too burdensome.

What do you think?

@evolve75
Copy link
Owner

@evolve75 evolve75 commented Dec 23, 2012

Mark,

This sounds good. Can you send the pull request? You might want to merge in my latest changes to the master first.

@evolve75
Copy link
Owner

@evolve75 evolve75 commented Feb 22, 2013

Mark,

I am about to make a Rubytree release this weekend. Would it be possible for you to send me a pull request for the hash export/import code that you have created?

@markthomas
Copy link
Author

@markthomas markthomas commented Feb 22, 2013

I'll review it today and let you know if it's ready.
On Feb 21, 2013 8:52 PM, "Anupam Sengupta" notifications@github.com wrote:

Mark,

I am about to make a Rubytree release this weekend. Would it be possible
for you to send me a pull request for the hash export/import code that you
have created?


Reply to this email directly or view it on GitHubhttps://github.com//issues/13#issuecomment-13924503.

@markthomas
Copy link
Author

@markthomas markthomas commented Feb 25, 2013

Unfortunately, there is still a small amount of work for me to do. I was
going to simplify it (make it just a node operation rather than a class
method on Tree, as the two method signatures would be different and
possibly confusing), and add a few more tests. I'll try to get to it this
week (just started a new job, so not sure at this point exactly what my
schedule will look like).

On Fri, Feb 22, 2013 at 6:07 AM, Mark Thomas mark.66.thomas@gmail.comwrote:

I'll review it today and let you know if it's ready.
On Feb 21, 2013 8:52 PM, "Anupam Sengupta" notifications@github.com
wrote:

Mark,

I am about to make a Rubytree release this weekend. Would it be possible
for you to send me a pull request for the hash export/import code that you
have created?


Reply to this email directly or view it on GitHubhttps://github.com//issues/13#issuecomment-13924503.

@evolve75
Copy link
Owner

@evolve75 evolve75 commented Oct 25, 2014

Feature request completed. Thanks to @jhamon for providing the feature code and patch.

@evolve75 evolve75 closed this Oct 25, 2014
@joemsak
Copy link

@joemsak joemsak commented Apr 23, 2020

So does this feature only work if all the final values are nil or a hash then? What about a real-world hash with data and content in it?

For example

Tree::TreeNode.from_hash( {"en"=>
  {"courses"=>"Courses",
   "layouts"=>{"navigation"=>{"desktop"=>{"browse_courses"=>"Browse Courses", "this_one"=>false}}},
   "unrelated"=>{"dont"=>{"include"=>"this, please"}}}})

#=> ArgumentError: Invalid child. Must be nil or hash.
#=> from /app/RubyTree/lib/tree/utils/hash_converter.rb:106:in `from_hash'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.