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

fix: change order of JSON Schema to search mapper transformations #32

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

ceberam
Copy link
Collaborator

@ceberam ceberam commented Sep 25, 2024

  • In the JSON Schema to search mapper, the suppression flags need to be addressed first, otherwise the jsonref.replace_refs function may remove them.
  • Updating pydantic from 2.8.2 to 2.9.2 triggers a change in JSON Schema from models: the lists with only 1 element get flattened. The documentation is automatically updated.

In the JSON Schema to search mapper, the suppression flags need to be addressed first,
otherwise the jsonref.replace_refs function may remove them.

Signed-off-by: Cesar Berrospi Ramis <75900930+ceberam@users.noreply.github.com>
Updating pydantic from 2.8.2 to 2.9.2 triggers a change in JSON Schema from models:
the  lists with only 1 element get flatten.

Signed-off-by: Cesar Berrospi Ramis <75900930+ceberam@users.noreply.github.com>
dolfim-ibm
dolfim-ibm previously approved these changes Sep 25, 2024
Copy link
Contributor

@dolfim-ibm dolfim-ibm left a comment

Choose a reason for hiding this comment

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

LGTM. Let's sync in which order we want to merge.

@ceberam
Copy link
Collaborator Author

ceberam commented Sep 25, 2024

LGTM. Let's sync in which order we want to merge.

This PR should be merged first, since the current version of pydantic (2.9.2) breaks main.

@@ -51,7 +51,7 @@ def test_json_schema_to_search_mapper_0():
index_ref = _load(filename)

diff = jsondiff.diff(index_ref, index_def)
print(json.dumps(index_def, indent=2))
# print(json.dumps(index_def, indent=2))
print(diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we comment out the diff

@@ -51,7 +51,7 @@ def test_json_schema_to_search_mapper_0():
index_ref = _load(filename)

diff = jsondiff.diff(index_ref, index_def)
print(json.dumps(index_def, indent=2))
# print(json.dumps(index_def, indent=2))
print(diff)
assert index_def == index_ref
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a proper error message in case this assert fails

assert index_def == index_ref, "assert index_def != index_ref"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case of assertion failure, pytest should already provide enough context information but nevertheless I added a commit with more verbose, including the difference.

Copy link
Contributor

@PeterStaar-IBM PeterStaar-IBM left a comment

Choose a reason for hiding this comment

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

see comments

Signed-off-by: Cesar Berrospi Ramis <75900930+ceberam@users.noreply.github.com>
Copy link
Contributor

@PeterStaar-IBM PeterStaar-IBM left a comment

Choose a reason for hiding this comment

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

nice!

@ceberam ceberam merged commit a4ddd14 into main Sep 26, 2024
5 checks passed
@ceberam ceberam deleted the fix/json-schema-mapper branch September 26, 2024 11:49
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.

4 participants