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.

Sign up for GitHub

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

Jump to bottom

Support toml v0.5.0 #122

Merged
merged 7 commits into from
Oct 25, 2019
Merged

Support toml v0.5.0 #122

merged 7 commits into from
Oct 25, 2019

Conversation

yassun7010
Copy link
Contributor

For toml v0.5.0 release,
I posted a pull request for a partial support before(see #116 ).

This time, I have made complete support and tests for toml v0.5.0.
Please consider it.

Change points

  • Add dotted keys.
  • Add hex, octal, and binary integer formats.
  • Add special float values (inf, nan)
  • Rename Datetime to Offset Date-Time.
  • Add Local Date-Time.
  • Add Local Date.
  • Add Local Time.
  • Allow space (instead of T) to separate date and time in Date-Time.

houndci-bot
houndci-bot reviewed Oct 11, 2019
View reviewed changes
test/toml_test.rb
assert_equal hash['array'], parsed['array']
assert_equal hash['table'], parsed['table']
assert_equal hash['inline-table'], parsed['inline-table']
assert_equal hash['array-of-tables'], parsed['array-of-tables']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['local-time'], parsed['local-time']
assert_equal hash['array'], parsed['array']
assert_equal hash['table'], parsed['table']
assert_equal hash['inline-table'], parsed['inline-table']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['local-date'], parsed['local-date']
assert_equal hash['local-time'], parsed['local-time']
assert_equal hash['array'], parsed['array']
assert_equal hash['table'], parsed['table']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['local-date-time'], parsed['local-date-time']
assert_equal hash['local-date'], parsed['local-date']
assert_equal hash['local-time'], parsed['local-time']
assert_equal hash['array'], parsed['array']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['offset-date-time'], parsed['offset-date-time']
assert_equal hash['local-date-time'], parsed['local-date-time']
assert_equal hash['local-date'], parsed['local-date']
assert_equal hash['local-time'], parsed['local-time']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
parsed = TomlRB.load_file(path)
hash = TomlRB::Examples.example_v_0_5_0
assert_equal hash['keys'], parsed['keys']
assert_equal hash['string'], parsed['string']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
path = File.join(File.dirname(__FILE__), 'example-v0.5.0.toml')
parsed = TomlRB.load_file(path)
hash = TomlRB::Examples.example_v_0_5_0
assert_equal hash['keys'], parsed['keys']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['products'], parsed['products']
assert_equal hash['fruit'], parsed['fruit']
end

def test_file_v_0_5_0
path = File.join(File.dirname(__FILE__), 'example-v0.5.0.toml')
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['float'], parsed['float']
assert_equal hash['integer'], parsed['integer']
assert_equal hash['string'], parsed['string']
assert_equal hash['table'], parsed['table']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

test/toml_test.rb
assert_equal hash['datetime'], parsed['datetime']
assert_equal hash['float'], parsed['float']
assert_equal hash['integer'], parsed['integer']
assert_equal hash['string'], parsed['string']
Copy link

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

emancu
emancu reviewed Oct 12, 2019
View reviewed changes
Copy link
Owner

@emancu emancu left a comment

Choose a reason for hiding this comment

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

Thank you so much for your changes. I expressed some concerns (the only important one is the .first) but in general it looks great.
If you can explain me how this won't break backwards compatibility, or if it does, how it would be great and we can put that on the release.

test/grammar_test.rb
@@ -9,37 +9,37 @@ def test_comment

def test_key
match = TomlRB::Document.parse('bad_key-', root: :key)
assert_equal('bad_key-', match.value)
assert_equal('bad_key-', match.value.first)
Copy link
Owner

Choose a reason for hiding this comment

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

I am a little concerned about this, because it is breaking backwards compatibility.

Copy link
Owner

Choose a reason for hiding this comment

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

Or I am getting something wrong from this changes? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To simplify the implementation of the dotted key, single key is regarded as a dotted key with one element(Table key is the same).

Certainly, if backwards compatibility is important,
I think it is necessary to separate single key and dotted key.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I would love to keep backwards compatibility but I also understand that is not a requirement since TOML is not necessary backwards compatible until it reaches 1.0.

So I'm happy to leave this as is, until someone reports a bug.

lib/toml-rb/keyvalue.rb
result[key k] = visit_value v
dotted_keys_str = @dotted_keys.join(".")
keys = symbolize_keys ? @dotted_keys.map(&:to_sym) : @dotted_keys
update = keys.reverse.inject(visit_value @value) { |k1, k2| { k2 => k1 } }
Copy link
Owner

Choose a reason for hiding this comment

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

This is not very clear, would you mind to add a comment here explaining 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.

For dotted keys support, dotted key string array is converted to a nested hash.
The value data is wrapped in the last key, and the previous keys are wrapped in reverse order.

Please refer to this example.

["a", "b", "c"].reverse.inject("value") {|k1, k2| { k2 => k1}}
=> {"a"=>{"b"=>{"c"=>"value"}}}

lib/toml-rb/keyvalue.rb Show resolved Hide resolved
lib/toml-rb/table.rb
class Keygroup
def initialize(nested_keys)
@nested_keys = nested_keys
class Table
Copy link
Owner

Choose a reason for hiding this comment

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

👍

lib/toml-rb/parser.rb
def visit_keygroup(keygroup)
@current = keygroup.navigate_keys @hash, @visited_keys, @symbolize_keys
def visit_table(table)
@fully_defined_keys = []
Copy link
Owner

Choose a reason for hiding this comment

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

Who uses this variable?

Copy link
Contributor Author

@yassun7010 yassun7010 Oct 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@fully_defined_keys is used as a memory for inline table at Keyvalue#assign.
Inline tables can not be used to add keys or sub-tables to an already-defined table.
See https://github.com/toml-lang/toml#inline-table

So, I add this variable, and tests for this problem.
test/examples/invalid/inline-table-duplicate-key.toml
test/examples/invalid/inline-table-duplicate-key2.toml

@emancu
Copy link
Owner

emancu commented Oct 22, 2019

We can drop the Ruby 2.2 version and that will make travis pass. Would you mind to update .travis.yml removing that version but adding 2.6 as well?

That would be the last change and we can merge this PR and release a new version

@yassun7010
Copy link
Contributor Author

Yes, please to add 2.6, thanks.

@emancu
Copy link
Owner

emancu commented Oct 25, 2019

@yassun4dev I just pushed the update to .travis.yml, yo uneed to rebase on that change

@emancu emancu merged commit b6fbafe into emancu:master Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emancu emancu emancu left review comments

@houndci-bot houndci-bot houndci-bot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yassun7010 @emancu @houndci-bot