Skip to content

convert nil to empty string#28

Merged
parkr merged 2 commits into
jm:masterfrom
rjocoleman:nil
Feb 17, 2014
Merged

convert nil to empty string#28
parkr merged 2 commits into
jm:masterfrom
rjocoleman:nil

Conversation

@rjocoleman

Copy link
Copy Markdown
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

parkr commented Feb 17, 2014

Copy link
Copy Markdown
Collaborator

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

@rjocoleman

Copy link
Copy Markdown
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

parkr commented Feb 17, 2014

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

parkr commented Feb 17, 2014

Copy link
Copy Markdown
Collaborator

Ok, cool. Thanks! 👍

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

@rjocoleman

Copy link
Copy Markdown
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

parkr commented Feb 17, 2014

Copy link
Copy Markdown
Collaborator

Awesome! LGTM. @jm?

@jm

jm commented Feb 17, 2014

Copy link
Copy Markdown
Owner

👍 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