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

Fix #47 and #70 #71

Merged
merged 15 commits into from
Jun 9, 2015
Merged

Fix #47 and #70 #71

merged 15 commits into from
Jun 9, 2015

Conversation

timotheeguerin
Copy link
Contributor

I added a few features trying to fix #47 and #70.

  • Subclasses the 2 error classes with TOML::Errror (Create TOML::Error as a base class for all errors #70)
  • Added a list of valid file to be parsed with toml and the json equivalent. Unit test is testing against each file.
  • This lead to a few unsupported feature:
    • Unicode escape characters(\u03B4)
    • Empty multiline expression(""""""")
  • I also replaced Unit::Test with Minitest::Test which i think is now recommended over Unit::Test
  • The error ValueOverwriteError was too strict. The following was failing to parse but is suppose to work:
[a.b.c]
answer = 42

[a]
better = 43
  • Having spaces in keys withour quotes is supposed to raise an error [invalid key] ["valid key"] so modified the grammar to be more strict. (check test/examples/invalid/table-whitespace.toml)

If anything seems wrong, tell me and I will fix it.

@@ -30,9 +30,14 @@ def test_keygroup
assert_equal(%w(owner.emancu),
match.value.instance_variable_get('@nested_keys'))

match = TOML::Document.parse('[ owner . emancu ]', root: :keygroup)
assert_equal(%w(owner emancu),

Choose a reason for hiding this comment

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

Extra blank line detected.

@@ -1 +1,2 @@
citrus -v 3.0.2
minitest -v 5.7.0
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you say that minitest is better ?
Honestly I didn't use minitest because I wanted to keep simple the gems dependencies even for dev.

Anyways, it's ok, I just wonder why did you need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look on the unit::test documentation http://ruby-doc.org/stdlib-2.1.1/libdoc/test/unit/rdoc/Test/Unit.html it state If you are writing new test code, please use MiniTest instead of Test::Unit.

Copy link
Owner

Choose a reason for hiding this comment

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

You have a good point. 👍

@emancu
Copy link
Owner

emancu commented Jun 8, 2015

Awesome PR!!

Thank you very much for taking care of those issues, I was a little busy to do it.

Let me know when its ready to be merged.

@timotheeguerin
Copy link
Contributor Author

On the other hand keeping test_grammar allow to test more specific case. Also I could not test the datetime parsing automaticaly with json comparing as json doesn't support it so we need to keep test_datetime at least. I can look into refactoring and removing duplicates after.

@timotheeguerin
Copy link
Contributor Author

Also I was wondering why you where using dep instead of bundler as they will only be used for contributing and most people use bundler anyway.

@emancu
Copy link
Owner

emancu commented Jun 8, 2015

I'm ok leaving test_grammar for specific cases.

In the other hand, about bundler let me suggest you a blog post: http://bits.citrusbyte.com/managing-ruby-dependencies

tl;dr Bundler is a very big dependency that solves a lot of problems that I don’t have

@@ -1,7 +1,9 @@
require_relative 'helper'
require_relative 'toml_examples'
require 'json'
require 'pp'
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i forgot to remove pp it i was trying to have a better compare view of the json and toml in case they differ but pp doesn't seem to improve much.

And i need json to parse the json file

@timotheeguerin
Copy link
Contributor Author

Thanks for the article, I see your point.

@emancu
Copy link
Owner

emancu commented Jun 8, 2015

Is it ready for merge ?

On Monday, June 8, 2015, Timothee Guerin [email protected] wrote:

Thanks for the article, I see your point.


Reply to this email directly or view it on GitHub
#71 (comment).

Emiliano Mancuso http://about.me/emancu

@timotheeguerin
Copy link
Contributor Author

It should unless you need anything else

emancu added a commit that referenced this pull request Jun 9, 2015
@emancu emancu merged commit 8bb1195 into emancu:master Jun 9, 2015
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.

Refactor tests
3 participants