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

Make strict schema #1480

Draft
wants to merge 33 commits into
base: 1.2-dev
Choose a base branch
from
Draft

Make strict schema #1480

wants to merge 33 commits into from

Conversation

duncandewhurst
Copy link
Contributor

@duncandewhurst duncandewhurst commented Feb 10, 2022

Related to #1046

Depends on open-contracting/ocdskit#184

Related issues from 1.2.0 Strict milestone:

TO-DO:

  • Remove deprecated types
  • Decide how to keep the strict schema up to date with changes to the regular schema
  • Update strict schema to match 1.2-dev
  • Update requirements.txt once schema-strict: add more validation keywords ocdskit#184 is released on PyPI
  • Add documentation (To be done in a separate PR)
  • Update changelog
  • Create a compiled-release-schema.json and remove "null" types and null enums from all fields.

@duncandewhurst duncandewhurst added this to the 1.2.0 Strict milestone Feb 10, 2022
@duncandewhurst
Copy link
Contributor Author

Decide how to keep the strict schema up to date with changes to the regular schema

@jpmckinney - so far I made the changes in this PR manually, but in order to keep the strict schema up to date, I can add a script as part of manage.py pre-commit to derive the strict schema programmatically. Does that sound good?

@jpmckinney
Copy link
Member

Yes, let's do it that way. Then we can run the script and see if the result is currently the same as your manual preparation :)

@duncandewhurst
Copy link
Contributor Author

Unfortunately, I'd made the manual changes to an old version of the schema and since merged the latest version of 1.2-dev so it's easier to just to diff the regular and strict schemas to check that the changes are as expected:

git diff --word-diff=color --no-index schema/release-schema.json schema/strict/release-schema.json
git diff --word-diff=color --no-index schema/release-package-schema.json schema/strict/release-package-schema.json
git diff --word-diff=color --no-index schema/record-package-schema.json schema/strict/record-package-schema.json

@duncandewhurst
Copy link
Contributor Author

The tests are failing on an unrelated issue from the organization units example and on the schema file for the new UnitValue definition (suggested in #881). I'm not sure if there's a better way to add that programmatically. Hardcoding it in manage.py seemed like a bad idea.

@jpmckinney
Copy link
Member

I'm not sure if there's a better way to add that programmatically.

Add what? The UnitValue definition? I figure we can add it to the regular schema (without extra validation keywords), and then just add the validation keywords in the strict schema.

@duncandewhurst
Copy link
Contributor Author

I'm not sure if there's a better way to add that programmatically.

Add what? The UnitValue definition? I figure we can add it to the regular schema (without extra validation keywords), and then just add the validation keywords in the strict schema.

I've added the UnitValue definition to the regular schema and updated manage.py to set the $ref for Unit.Value in the strict schema.

Do you have a view on if and how we should feature the strict schema(s) in the documentation?

I figure few users would do anything with the actual JSON schema, but adding schema viewers for the strict versions of the release, release package and record package schemas seems overly duplicative. I think we should include some mention of it in the docs if we are going to include it in the DRT and helpdesk feedback processes. We could either add an admonition to https://standard.open-contracting.org/staging/1.2-dev/en/schema/# or a new page under the reference section which can give a summary of the differences to the regular schema and direct users to the DRT to check their data against the strict schema.

@jpmckinney
Copy link
Member

Do you have a view on if and how we should feature the strict schema(s) in the documentation?

I think a concise admonition as you suggest would be best. It should briefly describe why the strict schema is preferred for new implementations (and how it can be useful to improve data quality of existing implementations).

In terms of duplication, we can consider using https://sphinx-panels.readthedocs.io/en/latest/ to allow browsing of each schema. That way it's not taking up a lot of the page.

@duncandewhurst
Copy link
Contributor Author

In terms of duplication, we can consider using https://sphinx-panels.readthedocs.io/en/latest/ to allow browsing of each schema. That way it's not taking up a lot of the page.

I tried this approach but only the first docson widget is displayed. The second widget throws an Uncaught Error: only one instance of babel-polyfill is allowed (see lbovet/docson#73).

In other projects that have multiple docson widgets per page we use https://github.com/OpenDataServices/docson which is a fork of an earlier version of docson that doesn't use babel-polyfill, but I see that https://github.com/open-contracting/docson makes several improvements that would be lost if we used the other fork.

I found https://www.npmjs.com/package/idempotent-babel-polyfill/ which might be a solution but my Javascript knowledge falls short of how to update docson to use it.

@duncandewhurst
Copy link
Contributor Author

@jpmckinney based on my previous comment, do you still want to use Sphinx Panels?

@duncandewhurst
Copy link
Contributor Author

In other projects that have multiple docson widgets per page we use https://github.com/OpenDataServices/docson which is a fork of an earlier version of docson that doesn't use babel-polyfill

Are you sure? I looked at several branches at https://github.com/OpenDataServices/docson/branches, comparing them to the source repository, and I don't see any removal of babel-polyfill.

The ODS fork doesn't remove babel-polyfill. Rather, babel-polyfill was added to the original docson repo after the ODS repo was forked and before the open-contracting repo was forked.

In OpenDataServices/docson@master...lbovet:docson:master there is a commit from the original repo that adds babel-polyfill: OpenDataServices/docson@0756db7, which also appears in the open-contracting fork: open-contracting/docson@0756db7

Anyway, let's do this in different PRs:

1. Make the changes for `manage.py` and `UnitValue`.

These changes were already made in this PR: 85af27b. Is that OK?

2. Start drafting the changes to the documentation. When ready to merge, run the `manage.py` command. As part of this, I can sort out removing the polyfill, which I think is not necessary for today's browsers.

So create a new PR for the documentation changes?

@jpmckinney
Copy link
Member

Noting that sphinx-panels is obsolete. Need to use https://github.com/executablebooks/sphinx-design.

@jpmckinney
Copy link
Member

To clarify my earlier request in #1480 (comment)

Let's try to get something merged sooner than later.

To do that, let's not generate the strict files in this PR. You can remove those files, and I'll "squash and merge". That means this PR is part 1 (manage.py and UnitValue).

In a new PR, we can add the documentation (the last checklist item). Once the docs are ready, we can run the manage.py command to generate the strict files, and then merge.

@jpmckinney
Copy link
Member

Note to myself that I haven't actually reviewed the changes yet, as I'm not sure if the PR is only "draft" due to the missing documentation changes.

@duncandewhurst
Copy link
Contributor Author

Let's try to get something merged sooner than later.

Erm... I'm not sure how I missed this one, sorry! It's now ready for review. I included a changelog entry for UnitValue even though it makes no difference to users. Feel free to remove, or we can just update it once the part 2 PR is done.

@duncandewhurst
Copy link
Contributor Author

duncandewhurst commented Oct 23, 2024

I've updated this PR to reflect the record schema being moved into its own file and to reflect the changes made to manage.py on 1.2-dev.

Edit: I also temporarily removed ruff-format from .pre-commit-config.yaml. It can be reinstated once #1708 (comment) is done.

@duncandewhurst duncandewhurst mentioned this pull request Oct 24, 2024
2 tasks
@odscjen
Copy link
Contributor

odscjen commented Nov 5, 2024

Added a new to-do from #330 (comment)

@duncandewhurst duncandewhurst marked this pull request as draft November 5, 2024 19:23
@duncandewhurst duncandewhurst removed the request for review from jpmckinney November 5, 2024 19:23
@duncandewhurst
Copy link
Contributor Author

Have moved back to draft whilst I make those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review in progress
Development

Successfully merging this pull request may close these issues.

3 participants