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

yaml library parses demes named "y" as booleans with the value TRUE #10

Closed
grahamgower opened this issue Mar 8, 2023 · 9 comments
Closed

Comments

@grahamgower
Copy link
Member

grahamgower commented Mar 8, 2023

Four valid test cases from the demes-spec repo (listed below) cannot be parsed because they contain a deme with the name "y". The yaml library only supports yaml v1.1 (not v1.2), and thus interprets these deme names as being a boolean type with the value TRUE. See #9.

toplevel_defaults_deme_01.yaml
successors_predecessors_01.yaml
deme_end_time_01.yaml
@reedacartwright
Copy link

I believe that tags or strings can be use to override this behavior, and this should be noted in the documentation.

@IsabelMarleen
Copy link
Collaborator

Minor correction, unicode_deme_name_04.yaml could not be parsed in #9 because of an unrelated unicode issue, see #11.

I believe that tags or strings can be use to override this behavior, and this should be noted in the documentation.

Do you mean specifying a custom handling function in the yaml parser? Part of the issue is that the values are not quoted, so in the yaml file it just says - name: y. According to this issue vubiostat/r-yaml#122 on the yaml parsing library repo, the value being unquoted means that it cannot be fixed without compiler modifications.

@grahamgower
Copy link
Member Author

Minor correction, unicode_deme_name_04.yaml could not be parsed in #9 because of an unrelated unicode issue, see #11.

Ah, yep, thanks. I removed it from the list.

I believe that tags or strings can be use to override this behavior, and this should be noted in the documentation.

Do you mean specifying a custom handling function in the yaml parser? Part of the issue is that the values are not quoted, so in the yaml file it just says - name: y. According to this issue vubiostat/r-yaml#122 on the yaml parsing library repo, the value being unquoted means that it cannot be fixed without compiler modifications.

No, I think Reed means that one can write !!str in the yaml file to force the type to be a string. E.g.

time_units: generations
demes:
 - name: !!str y
   epochs:
    - start_size: 1000

It would be nice if we could make this work by somehow telling the yaml parser that the name field of a deme is always a string, but I'm not sure that's possible. Maybe the handler arg to the yaml.load() function could be used somehow?

@reedacartwright
Copy link

Given that y can be interpreted in the standard as a boolean, and name is supposed to be a string, it seems to me that the input test file is the problem, not the library. The demes file should look something like this:

time_units: generations
demes:
 - name: "y"
   epochs:
    - start_size: 1000

@grahamgower
Copy link
Member Author

Given that y can be interpreted in the standard as a boolean, and name is supposed to be a string, it seems to me that the input test file is the problem, not the library.

Yes I think you're right that we should fix the test files. It's a shame that YAML 1.2 parsers are not widespread, given that it's now 14 years old.

@IsabelMarleen
Copy link
Collaborator

Alright, then I'll implement a custom handling function for "n" and "y", so that it works when the test files have changed, and then I'll close this issue.

Thank you both for your help!

@grahamgower
Copy link
Member Author

Oh, you shouldn't need to have a custom handler. When the source file has the y in quotes, like name: "y", this will be interpreted as a string in YAML v1.1.

@grahamgower
Copy link
Member Author

These test files have now been fixed in the spec repository. Updating to use the latest spec test files here should close this issue.

@IsabelMarleen
Copy link
Collaborator

I just updated to the latest spec test files in Pull Request #13 and will close this issue now.

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

No branches or pull requests

3 participants