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

feat: add json schema #8294

Merged
merged 3 commits into from Jun 30, 2021
Merged

feat: add json schema #8294

merged 3 commits into from Jun 30, 2021

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Jun 28, 2021

Assuming I've understood the YAML Schema correctly (I did read the Yamale README), I believe they should be equivalent (or at least almost equivalent.)

I made a new PR instead of reviewing an older one, as I felt there was a lot to change.

I've tested this against the following:

Note: Before testing I manually changed all python.version values to 3.6 as original values (3.7/3.8) aren't valid against the schema. (Maybe consider using string rather than enum here?)

Let me know if anything looks off to you!

Subtle Differences

  • For paths, it just accepts any string. I could validate it against a regex? I thought maybe just leaving it as any string would be fine.

@SethFalco SethFalco force-pushed the json-schema branch 2 times, most recently from 8355a94 to 099ead0 Compare June 28, 2021 23:28
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Thanks for taking over this!

readthedocs/rtd_tests/fixtures/spec/v2/schema.json Outdated Show resolved Hide resolved
readthedocs/rtd_tests/fixtures/spec/v2/schema.json Outdated Show resolved Hide resolved
readthedocs/rtd_tests/fixtures/spec/v2/schema.json Outdated Show resolved Hide resolved
@SethFalco
Copy link
Contributor Author

SethFalco commented Jun 30, 2021

Redirects aren't implemented yet, either.

I interpreted that as "take that same action as with pip file support".
Let me know if that was wrong of me!

Just noting the dropped sections here for future reference.

Pipfile (properties.python.properties.install.items.anyOf[2])

{
  "properties": {
    "pipfile": {
      "title": "Pipfile",
      "description": "The path to the directory that contains the Pipfile.",
      "type": "string"
    },
    "dev": {
      "title": "Dev",
      "description": "Add the --dev option to pipenv install.",
      "type": "boolean",
      "default": false
    },
    "ignore_pipfile": {
      "title": "Ignore Pipfile",
      "description": "Add the --ignore-pipfile option to pipenv install.",
      "type": "boolean",
      "default": true
    },
    "skip_lock": {
      "title": "Skip Lock",
      "description": "Add the --skip-lock option to pipenv install.",
      "type": "boolean",
      "default": true
    }
  },
  "required": [
    "pipfile"
  ]
}

Redirects (properties.redirects)

"redirects": {
  "title": "Redirects",
  "description": "Redirects for the current version to be built",
  "properties": {
    "page": {
      "additionalProperties": {
        "type": "string"
      }
    }
  },
  "default": null
}

@stsewd
Copy link
Member

stsewd commented Jun 30, 2021

I interpreted that as "take that same action as with pip file support".

yes, that was it

Co-authored-by: Santos Gallegos <stsewd@protonmail.com>
@SethFalco
Copy link
Contributor Author

SethFalco commented Jun 30, 2021

I did the things you asked for , plus one additional change:

Since version only has one possible value, and it's a required field anyway, I figured it might as well also have a default configured to "2" as well.

Would you care to give an opinion on that?

Can I also just confirm, did I reference the correct YAML Schema when I created the JSON Schema? 🤔 (I used this one.)
As in, are your suggestions fixing mistakes of mine, or are they fresh/new changes to keep things up to date?

Does the YAML Schema need to be updated separately as well?

@stsewd
Copy link
Member

stsewd commented Jun 30, 2021

Since version only has one possible value, and it's a required field anyway, I figured it might as well also have a default configured to "2" as well.

We do allow 1, which is the default to use if you don't set a version, so it should be required.

Can I also just confirm, did I reference the correct YAML Schema when I created the JSON Schema? thinking (I used this one.)
As in, are your suggestions fixing mistakes of mine, or are they fresh/new changes to keep things up to date?

Yeah, that scheme is the one, but it has some ideas that were never implemented, we may remove it and use this json scheme instead of yamale, so no need to update it here.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Thanks!

@stsewd stsewd merged commit 0e9dcaf into readthedocs:master Jun 30, 2021
@stsewd
Copy link
Member

stsewd commented Jun 30, 2021

@SethFalco let us know if you need anything from us to submit this to https://github.com/SchemaStore/schemastore/

@SethFalco
Copy link
Contributor Author

SethFalco commented Jun 30, 2021

Thanks for that! I think it's simple, so no need to worry.
Just have to add it a reference to the file merged to the catalog.json file in the repo.

{
  "name": "Read the Docs",
  "description": "Read the Docs configuration file",
  "fileMatch": [
    "readthedocs.yml",
    "readthedocs.yaml",
    ".readthedocs.yml",
    ".readthedocs.yaml"
  ],
  "url": "https://raw.githubusercontent.com/readthedocs/readthedocs.org/master/readthedocs/rtd_tests/fixtures/spec/v2/schema.json"
}

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.

Publish configuration file schema to http://schemastore.org
2 participants