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

Tests refactor #2

Closed
wants to merge 4 commits into from
Closed

Conversation

davidfischer
Copy link
Contributor

This breaks the toml files used for tests into separate files. This way other projects might be able to use those files without having to extract them from the Python tests. I had created these tests for my own pytoml implementation based on pyparsing. However, when I saw yours, I figured I'd just integrate my tests into your project.

These tests uncovered what I believe to be a couple bugs:

  • Keys can start with or consist entirely of numbers (see #65). This hasn't entirely been decided but it will be good to have tests either way.
  • Single quoted strings cannot contain escaped single quotes and the same for double quotes.

@davidfischer
Copy link
Contributor Author

Somehow I missed in my reading of the spec that there are no single quoted strings. However, double quoted strings cannot contain escaped double quotes.

@davidfischer
Copy link
Contributor Author

The double quoted strings issue is fixed in 0b33768 after #4.

@bryant
Copy link
Owner

bryant commented Feb 26, 2013

I placed your changes in branch and went ahead to refactor some more. It's here. Let me what you think before I merge it into master.

@davidfischer
Copy link
Contributor Author

I love the refactor! I figured we could do something like this but I was worried it would collapse the tests into one single large test. This code is quite elegant. Who doesn't love a good eval?

The only issue I see is the comment I made about using dirname with abspath. I believe Windows (although I can't readily verify it) has issues with dirname without abspath.

@davidfischer
Copy link
Contributor Author

If eval is a concern, we could use a datetime aware JSON parser.

@bryant
Copy link
Owner

bryant commented Feb 26, 2013

Merging.

@bryant bryant closed this Feb 27, 2013
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.

2 participants