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 support for single nested hashes. #36

Closed
wants to merge 1 commit into from
Closed

Conversation

asenchi
Copy link
Owner

@asenchi asenchi commented Aug 28, 2013

I'm actually curious about what people think of this, is it worth it?

@wuputah opened #25 months ago but at the time I wasn't sure it was useful, however, I've finally hit a point where it would be.

Obviously this is something that could get out of hand, I don't think Scrolls should ever be concerned with hashes nested more than one deep, but a single nested hash actually works out pretty nicely.

I've chosen the '.' as a hash separator for a couple of reasons:

  • In general keys should match \w+, so using a '_' as @wuputah suggests would become impossible to parse.
  • ':' is used in Time formats and should, I think, be left alone with regards to keys.
  • The TOML spec uses '.' for key groups so there is some precedence.

As an explanation here is what this PR gets us:

log_this = {
  "rid"  => "blah-blah-blah",
  "data" => {
    "req_method" => "get",
    "req_path"   => "/emoji"
  }
}

Parses to:

rid=blah-blah-blah data.req_method=get data.req_path=/emoji

@asenchi
Copy link
Owner Author

asenchi commented Aug 28, 2013

/cc @lstoll

@eric
Copy link

eric commented Aug 28, 2013

I think it seems reasonable. Why limit it to one level?

One thing that comes to mind is that this may be worth a configuration setting to enable or disable — there are times that it's nice to get things as nested hashes, but others where it's easier to just be able to say hash['data.req_method'] directly...

@danp
Copy link
Contributor

danp commented Aug 28, 2013

Not sure I would use this but it seems reasonable.

@wuputah
Copy link

wuputah commented Aug 28, 2013

:shipit:

@asenchi
Copy link
Owner Author

asenchi commented Aug 28, 2013

I think it seems reasonable. Why limit it to one level?

TBH, to discourage the practice. I think Scrolls should be rather opinionated about structure, perhaps that is naive?

Also, how far do we go? I think Scrolls should remain simple, and handling deeply nested hashes gets messy (admittedly the code I would write would be messy, there might be an elegant way to handle this).

I think by default we'll keep the previous behavior and toggle this behavior on. We can address if that is the proper default in a later point release.

@lstoll
Copy link

lstoll commented Aug 28, 2013

👍 to this.

Using a period as a separator makes sense, now that you mention it. I also support limiting it to one level of depth. Being able to export a series of keys in a category makes sense to me, exporting nested data structures makes for messy and hard to understand data.

@asenchi
Copy link
Owner Author

asenchi commented Sep 6, 2013

I've actually going to punt on this for the time being. I'll come back to it. I'm not quite convinced it's needed and want to clean up the implementation before I would consider adding it and I don't have a free minute currently.

@asenchi
Copy link
Owner Author

asenchi commented Aug 30, 2017

Going to punt fully and close this down. Maybe add something around this later.

@asenchi asenchi closed this Aug 30, 2017
@asenchi asenchi deleted the handle_nested_hashes branch August 30, 2017 02:32
@lstoll
Copy link

lstoll commented Aug 30, 2017

Blast from the past.

@asenchi
Copy link
Owner Author

asenchi commented Aug 30, 2017

Hah, indeed. ❤️

@asenchi asenchi mentioned this pull request Aug 31, 2017
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.

None yet

5 participants