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
reader: fix jgf properties #1149
reader: fix jgf properties #1149
Conversation
17e6eff
to
f49a102
Compare
f49a102
to
3da41f8
Compare
Problem: the JGF reader requires each vertex's 'properties' field to be either null or a JSON object, but doesn't ever check the type. This leads to silent failures when the type read in is wrong. Add a check for the type of the properties field if it is not null.
Problem: the Python JGF writer outputs properties as a list by default, but the reader expects a mapping. Change the default to a dictionary.
3da41f8
to
6e20c67
Compare
@milroy this is ready for review whenever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from a typo in one commit message this PR LGTM!
@@ -45,6 +45,17 @@ | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the commit message. I think you mean "there are no tests where the JGF writer handles properties correctly."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks! Force-pushed with the correction.
Problem: there are no tests that the JGF writer handles properties correctly. Add them.
Problem: t/t8001-util-ion-R.t expects JGF properties to be formatted as a JSON array, but they are actually given by a mapping. Fix the expected type.
6e20c67
to
f2f2899
Compare
Thanks! Setting MWP. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1149 +/- ##
========================================
+ Coverage 71.0% 73.9% +2.8%
========================================
Files 96 103 +7
Lines 12867 14395 +1528
========================================
+ Hits 9147 10643 +1496
- Misses 3720 3752 +32
|
Given this vertex in JGF, resource-query reports it has no properties.
Looking at the reader, I found that it expects properties to be given as a mapping with keys the name of the property. When I changed the vertex in JGF to the following, the property was recognized. It looks like the reader had been silently failing on the type.