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

Fixes #31 #36

Merged
merged 1 commit into from
Oct 1, 2014
Merged

Fixes #31 #36

merged 1 commit into from
Oct 1, 2014

Conversation

squaresurf
Copy link
Contributor

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
end
class String
def to_toml; self.inspect; end
def to_toml(path = ""); self.inspect; end
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
Contributor 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
Contributor 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
Contributor 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.

None yet

2 participants