Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Add tests from @iarna/toml-spec-tests in prescribed JSON format #8

Closed
wants to merge 10 commits into from

Conversation

pyrmont
Copy link

@pyrmont pyrmont commented Jan 22, 2021

Summary

This PR adds almost¹ all of the tests from @iarna's toml-spec-tests repository but:

  1. uses JSON rather than YAML for the output files;
  2. encodes the JSON in the format described in the JSON encoding document; and
  3. deletes the YAML files since they are no longer necessary.

All credit for writing the original TOML tests belongs to @iarna and the contributors to her repository.

Background

I created the tests for my own use in developing Tomlin, a TOML parser in Janet. For my purposes, I test and compare the results using a Janet script. As a result, I have not tested this with the Python script in this repository. Nevertheless, since the time-consuming work is in writing the JSON files in the prescribed format, I wanted to share what I'd done.

Notes

This PR conflicts with #5. While that PR was made first, given that it includes YAML output files, I would argue that this PR is to be preferred.

¹ The nested QA tests (here and here) break Janet's JSON encoder/decoder and I have not written JSON output files for them.

@pyrmont
Copy link
Author

pyrmont commented Jan 22, 2021

As part of the PR, I've also removed the mixed-line-ending and trailing-whitespace hooks from .pre-commit-config.yaml. These hooks conflict with tests that are checking how a TOML parser handles various characters.

@pradyunsg
Copy link
Member

Thanks for the PR! ^.^

Looks like I know what I'm reviewing all of Saturday morning!

@hukkin
Copy link

hukkin commented Jun 4, 2021

Thanks pyrmont, this was incredibly useful to me!

Here's two missing JSON files (note that these are NOT the nested tests that you mentioned, seems like in total 4 files were missing)

@hukkin
Copy link

hukkin commented Jun 4, 2021

Oh, and I've got one question about the encoding used for escaped characters: why are some characters double escaped in JSON?

E.g.

{"a":
  {"type":"string","value":"\\n"}}

The actual value we want is just a newline. However in the JSON we have a backslash plus a newline.

On the other hand we have

{"a":
  {"type":"string","value":"\""}}

Both the actual value and the JSON encoded value are just a single quote character.

This seemed inconsistent, but maybe I'm missing something obvious?

@pyrmont
Copy link
Author

pyrmont commented Jun 4, 2021

@hukkinj1 Thanks for the kind words and thanks for spotting the missing files! Not sure what happened there.

And you were right, I just completely screwed up the JSON encoding. To be honest, I'm still confused so I'm not 100% sure this is correct but I think you were right and that the escape sequences in the string values don't need to be escaped. I've fixed that now and it's hopefully correct!

@hukkin
Copy link

hukkin commented Jun 5, 2021

The escapes seem good to me now. Thanks once more!

@CAM-Gerlach
Copy link

Hey @pradyunsg have you gotten a chance to review this, or is there anything blocking this PR from being merged? It is specifically mentioned in @hukkin 's PEP 680 and elsewhere.

As part of the PR, I've also removed the mixed-line-ending and trailing-whitespace hooks from .pre-commit-config.yaml. These hooks conflict with tests that are checking how a TOML parser handles various characters.

FYI, instead of removing those hooks completely from the whole repo, you can exclude files by type (with exclude_types) or by regex (with exclude).

@@ -1,6 +1,6 @@
The MIT License

Copyright (c) 2020 Pradyun Gedam
Copyright (c) 2021 Pradyun Gedam and Contributors
Copy link

Choose a reason for hiding this comment

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

According to a strict reading of the MIT licence, the copyright notice should appear exactly as in the original, so something like:

Suggested change
Copyright (c) 2021 Pradyun Gedam and Contributors
Copyright (c) 2021 Pradyun Gedam and Contributors
Copyright Rebecca Turner <me@re-becca.org>

would be more appropriate than adding a new note at the bottom.

Copy link

@CAM-Gerlach CAM-Gerlach Mar 23, 2022

Choose a reason for hiding this comment

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

(I am not a licensed attorney, and the following should not be taken as formal legal advice.)

I concur with what you state above, as modifying the license text in this manner while not preserving the copyright notice intact, at its location in the original, is not an accepted approach for what is intended here.

To note, I do offer an alternate suggestion, as multiple copyright lines of text, rather than the single one matching the form of the one specified as variable in the SPDX definition of the MIT license, often breaks programmatic license detection and validation, including GitHub's. An alternative would be placing both complete copyright notices on the same line with clear lexical separation between them, like this:

Suggested change
Copyright (c) 2021 Pradyun Gedam and Contributors
Copyright Rebecca Turner <me@re-becca.org>; Copyright (c) 2021 Pradyun Gedam and Contributors

Given the original copyright notice is preserved and set off with a semicolon, both follow the form of standalone copyright notices, and this one line is permitted to vary without changing the legal effect of the license, it would seem equivalent separation is achieved without resorting to a line break.

@arp242
Copy link
Contributor

arp242 commented Jul 30, 2022

Hey @pradyunsg have you gotten a chance to review this, or is there anything blocking this PR from being merged?

AFAIK the status is that https://github.com/BurntSushi/toml-test is the de-facto standard: toml-lang/toml#585 (comment)

So ideally, toml-test should be moved here, overwriting this.

@arp242 arp242 mentioned this pull request Jul 31, 2022
@pyrmont
Copy link
Author

pyrmont commented Aug 2, 2022

I'm not sure what the delay has been but I'll close this PR as it's been outstanding for more than a year at this point and after a cursory examination, the tests in @BurntSushi's repository look more comprehensive.

@pyrmont pyrmont closed this Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants