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

Use UriKind.RelativeOrAbsolute to allow deserializing of relative URIs #401

Merged
merged 4 commits into from
Feb 22, 2022

Conversation

dermotblairca
Copy link
Contributor

Fixing issue where relative URIs in JSON are ignored and not included in the deserialized schema.org objects, as discussed in #391

@RehanSaeed
Copy link
Owner

RehanSaeed commented Feb 21, 2022

Thanks! Can we also add some tests?

@RehanSaeed RehanSaeed added bug Issues describing a bug or pull requests fixing a bug. patch Pull requests requiring a patch version update according to semantic versioning. labels Feb 21, 2022
@dermotblairca
Copy link
Contributor Author

Hi @RehanSaeed I have added a new test to check the relative URI scenario, and also updated an existing test to include a relative URI when deserializing an object of type Thing. It looks like the absolute URI scenarios are already sufficiently tested. However if you would like more test cases, let me know and I can add them. Thanks.

@RehanSaeed
Copy link
Owner

Thanks @dermotblairca. The ReadJson_Values_SingleValue_ThingInterface test seems to be failing though.

@dermotblairca
Copy link
Contributor Author

@RehanSaeed I can't actually reproduce that failed test. When I do a clean rebuild on my fork, all of the tests run fine including that one. For example:
image
Could it be environment-specific maybe?

Seeing if RelativeOrAbsolute fixes unit tests
Removing slash so it doesn't think it's a file on some environments.
@dermotblairca
Copy link
Contributor Author

Hopefully my last change has fixed the tests. I think when ToString() was implicitly called on the URI of actual/second argument on the Assert call, it seems to prepend it with "file://". I cannot reproduce this though so I'm guessing it only happens on some environments.

@RehanSaeed RehanSaeed merged commit f64670f into RehanSaeed:main Feb 22, 2022
@RehanSaeed
Copy link
Owner

Thanks!

@dermotblairca
Copy link
Contributor Author

No problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues describing a bug or pull requests fixing a bug. patch Pull requests requiring a patch version update according to semantic versioning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants