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

autocast number types #225

Merged
merged 8 commits into from
Jun 23, 2021
Merged

autocast number types #225

merged 8 commits into from
Jun 23, 2021

Conversation

jspaaks
Copy link
Member

@jspaaks jspaaks commented Jun 22, 2021

Related issues

Refs: #91

Describe the changes made in this pull request

from #91 (comment)

Now that we have switched to JSONSchema, we should re-evaluate whether union types for some keys are useful. For example, it is currently (https://github.com/citation-file-format/citation-file-format/blob/38d9b1fb629373f6a3607de64aa3a6e21a76f750/schema.json) invalid to write the following in CITATION.cff:

version: 1
version: 1.11
number: 41
issue: 11

This is a bit unexpected and inconvenient for people particularly those who write CITATION.cff files by hand / unassisted.

I went over the keys and tried to identify a list of keys that could potentially have values that are interpreted as numbers by YAML, which would render them invalid. Here's the list I came up with:

  1. #/definitions/post-code
  2. issue
  3. number
  4. reference/version
  5. section
  6. version

This PR expands their "type": "string" to a str|number union type using JSONSchema "anyOf".

This also meant that some of the tests were no longer necessary, specifically the tests that verify that certain CITATION.cff files do not pass validation. This PR removes those tests (from examples/1.2.0/fail/**)

Instructions to review the pull request

cd $(mktemp -d --tmpdir cff.XXXXXX)
git clone https://github.com/citation-file-format/citation-file-format .
git checkout 91-autocast-number-types
python3 -m venv env
source env/bin/activate
pip install --upgrade pip setuptools wheel
pip install -r requirements.txt
pytest

pytest should pass with 68 (not 70 like before) items discovered

@jspaaks jspaaks changed the base branch from main to 1.2.0 June 22, 2021 11:32
@jspaaks jspaaks marked this pull request as ready for review June 22, 2021 12:46
Copy link
Member

@sdruskat sdruskat left a comment

Choose a reason for hiding this comment

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

Think this needs a resolution of #91 per discussion before it can be reviewed, and may need more work after we've come to a decision there.

@jspaaks jspaaks marked this pull request as draft June 22, 2021 13:34
@jspaaks jspaaks requested a review from sdruskat June 22, 2021 17:42
Copy link
Member

@sdruskat sdruskat left a comment

Choose a reason for hiding this comment

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

Thanks, excellent work.
Should we perhaps deduplicate version via a definition?

@jspaaks
Copy link
Member Author

jspaaks commented Jun 23, 2021

Should we perhaps deduplicate version via a definition?

Created #226

@jspaaks jspaaks marked this pull request as ready for review June 23, 2021 06:52
@jspaaks
Copy link
Member Author

jspaaks commented Jun 23, 2021

Thanks for reviewing!

@sdruskat sdruskat merged commit 47372e6 into 1.2.0 Jun 23, 2021
@sdruskat sdruskat deleted the 91-autocast-number-types branch June 23, 2021 06:53
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.

None yet

2 participants