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

Calculate additional codelist values for schema using anyOf #137

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

odscjames
Copy link
Collaborator

Like OCDS record packages

#125 open-contracting/lib-cove-ocds#106

@odscjames odscjames self-assigned this Nov 8, 2023
@odscjames
Copy link
Collaborator Author

@jpmckinney I wanted to simplify the tests, so we didn't also have to change the check for local or remote schemas. Does this still capture the change you are after?

@jpmckinney
Copy link
Contributor

jpmckinney commented Nov 8, 2023

The integration.yml changes from #125 are relevant - otherwise these tests will forever fail.

The change to if schema_url.startswith("http") needs to be carried over. It's necessary to "fix(custom_jsonref_jsonloader): Read from filesystem if schema_url is a file path", because lib-cove has already messed around with the uri value by this point (IIRC), and checking schema_url directly is the only way to know if the URL is a file path or not.

Edit: Indeed - uri is parsed, and then uri is set to uri = urljoin(schema_url, ...). So, if schema_url is a file path, then "http" in uri_info.scheme will be True, even though the subsequent GET request will use a value of uri that's prefixed with a file path (i.e. no scheme). So, it's necessary to check schema_url instead of uri. This is more correct, even if this particular test doesn't need it.

As for the test, if it fails without the changes to common.py (and passes with the changes), then that's what matters.

@odscjames
Copy link
Collaborator Author

The integration.yml changes from #125 are relevant - otherwise these tests will forever fail.

It's on my todo list and I was going to look at separately

The change to if schema_url.startswith("http") ....

Noted in seperate issue to check out - thanks.

As for the test, if it fails without the changes to common.py (and passes with the changes), then that's what matters.

It does!

@odscjames
Copy link
Collaborator Author

@jpmckinney - @Bjwebb has pointed out the comments are anyOf but the code is oneOf - it should be doing this for both right? If so I can just add this and we should be done:

elif "anyOf" in value and isinstance(value["anyOf"], list):
            descendants = value["anyOf"]

@jpmckinney
Copy link
Contributor

I probably meant oneOf everywhere (OCDS doesn't use anyOf), but supporting anyOf should be the same logic, so that change sounds good.

@odscjames
Copy link
Collaborator Author

Added.

I don't think we need a special test just for that elif - if I change schema_with_oneof_codelists.json locally I can see it working for anyOf too.

But over to @Bjwebb to review!

@odscjames odscjames merged commit e7b4fa4 into main Nov 23, 2023
18 of 22 checks passed
@odscjames odscjames deleted the 2023-11-08 branch November 23, 2023 14:09
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.

3 participants