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

convert nil to empty string #28

Merged
merged 2 commits into from
Feb 17, 2014
Merged

convert nil to empty string #28

merged 2 commits into from
Feb 17, 2014

Conversation

rjocoleman
Copy link
Contributor

Currently if a key's value is nil when using TOML::Generator.new(hash).body a undefined method error is thrown: NoMethodError: undefined method 'to_toml' for nil:NilClass

This PR converts supports nil.to_toml by converting it into an empty string.

@parkr
Copy link
Collaborator

parkr commented Feb 17, 2014

Can you please show where in the spec we should expect nil to be "" once read in?

@rjocoleman
Copy link
Contributor Author

My interpretation of toml-lang/toml#30 is that a key with a nil value should not be present, my rational is that if it's present it should be handled rather than the current error and nil is functionally equivalent to "" in many cases - at least mine.

Perhaps a better solution would be:

other_pairs.each do |pair|
  key, val = pair
  if key.include? '.'
    raise SyntaxError, "Periods are not allowed in keys (failed on key: #{key.inspect})"
  end
  unless val.nil?
    @body += "#{key} = #{format(val)}\n"
  end
end

at https://github.com/jm/toml/blob/master/lib/toml/generator.rb#L54

@parkr
Copy link
Collaborator

parkr commented Feb 17, 2014

nil is functionally equivalent to "" in many cases

>> if nil
..   print "whee"
.. end
# nothing prints
>> if ""
..   print "whee"
.. end
"whee"

nil and "" don't look functionally equivalent to me. If anything, NilClass#to_toml should return itself. nil is falsey and "" is truthy so we need to respect that difference.

Perhaps a better solution would be ...

In toml-lang/toml#30, it looks like mojombo decided a key with no value should be an empty hash table, so we should set the value for keys without any value to be an empty hash table.

@rjocoleman
Copy link
Contributor Author

nil and "" don't look functionally equivalent to me. If anything, NilClass#to_toml should return itself. nil is falsey and "" is truthy so we need to respect that difference.

Agreed, I should have qualified my statement to be limited for uses as configuration where often nil and "" are treated the same. Opinions and differing use cases aside this NilClass#to_toml should be handled per the spec.

In toml-lang/toml#30, it looks like mojombo decided a key with no value should be an empty hash table, so we should set the value for keys without any value to be an empty hash table.

It was said that an empty key group should be an empty hash table. This term is defined in v0.1.0 and redefined as tables in v0.2.0

My following of the thread is still that a key with no value is omitted.

I've updated my PR to handle these.

require 'toml'

hash = {
  "bar" => nil,
  "foo" => {}
}

doc = TOML::Generator.new(hash).body
# => "\n[foo]\n"

@parkr
Copy link
Collaborator

parkr commented Feb 17, 2014

Ok, cool. Thanks! 👍

Can you please write a quick test for this? Thank you!

@rjocoleman
Copy link
Contributor Author

I've added the empty table and a nil value to the hash in test/test_generator.rb and then removed the nil value from the expected result..
I believe this covers it but I don't use minitest so I could be missing something.

@parkr
Copy link
Collaborator

parkr commented Feb 17, 2014

Awesome! LGTM. @jm?

@jm
Copy link
Owner

jm commented Feb 17, 2014

👍 Looks good to me!

parkr added a commit that referenced this pull request Feb 17, 2014
convert nil to empty string
@parkr parkr merged commit deeb624 into jm:master Feb 17, 2014
@parkr parkr added this to the 0.2.0 milestone Feb 17, 2014
@parkr parkr mentioned this pull request Feb 17, 2014
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.

3 participants