Skip to content

Conversation

@squaresurf
Copy link

This is a pretty hefty refactor. I found it easier to understand toml
generation with the majority of the generation code within the
monkey_patch rather than the generator class.

Please feel free to mark up my PR with comments as my ruby style may be a bit funky.

This is a pretty hefty refactor. I found it easier to understand toml
generation with the majority of the generation code within the
monkey_patch rather than the generator class.
@parkr parkr self-assigned this Sep 23, 2014
@parkr parkr added the fix label Sep 23, 2014
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that #to_toml accepts any args.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer that only Array.to_toml and Hash.to_toml accept path or do you not like that it defaults to an empty string?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to be the same as everyone else so it's expected behaviour. Does #to_json and similar converting methods accept paths? If so, can you link me to it? Do we need this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path is necessary so that we can pass along the parent keys for nested hashes. I can try and think of a replacement if you'd like.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah i guess it's ok to leave it as long as it has a default. thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks :)

parkr added a commit that referenced this pull request Oct 1, 2014
Support array of tables

Fixes #31
@parkr parkr merged commit 002a521 into jm:master Oct 1, 2014
@squaresurf squaresurf deleted the issue-31 branch October 1, 2014 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants