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

person-statement.json & ownership-or-control-statement.json - statementType should not have codelist options #375

Closed
ghost opened this issue Oct 29, 2021 · 3 comments

Comments

@ghost
Copy link

ghost commented Oct 29, 2021

Note each of the following, for statementType, has an enum with only one option and NO codelist options:

These ones, for statementType, has an enum with only one option but DO have codelist options:

Them having the code list options is a problem because when you compile the schema with compiletojsonschema, the tool sees the codelist options and then helpfully fills in the Enum with all the options. Then the oneOf mechanism that selects between the 3 statement types is broken and validation fails.

I see they were deliberately put back recently:

However I can't see any description for this change beyond "make it consistent", so I'm hoping we can remove it?

ghost pushed a commit to openownership/lib-cove-bods that referenced this issue Oct 29, 2021
@rhiaro
Copy link
Collaborator

rhiaro commented Oct 29, 2021

I think it's redundant to have the enum values in the json schema as well as in the codelist csvs. Now the compile tool fills in the enums automatically I can't think of a good reason to add them by hand to the json schema, so we can remove them.

In the statementType case we don't need the schema to point to the codelist at all, and can just use the fixed value (which will solve the problem for the compile tool).

The statementType.csv codelist is there for the purpose of documenting the types of statements available though. If we keep the csv file but remove the links to it from the schema, a test should fail. There's already a small tweak for statementType in the test, so we just need to move this tweak to skip statementType when testing all codelists are used instead.

@kd-ods
Copy link
Collaborator

kd-ods commented Nov 8, 2021

So would we use pattern to validate the value of statementType in each case?

Now the compile tool fills in the enums automatically I can't think of a good reason to add them by hand to the json schema, so we can remove them.

Would that break anything in the docs, or are the docs generated from the compiled schema anyway? (I think it was @odscjames who suggested that we add a compiled version of the schema to the repo. For the record, I agree. And its availability should be flagged in the docs.) Obviously these last points aren't directly related to this issue, so if we think we want to remove the enums, let's spin up another ticket asap.

@ghost
Copy link
Author

ghost commented Nov 9, 2021

So would we use pattern to validate the value of statementType in each case?

No, Enum - this is already in place and won't be changed by this work. The validation libraries are already looking for enum.

Created new issue for codelist values being duplicated: #380

Done pull request to fix this issue: #382

This issue was closed.
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

2 participants