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

Optional and Object for InterTypes #96

Merged
merged 6 commits into from
Jan 23, 2024
Merged

Optional and Object for InterTypes #96

merged 6 commits into from
Jan 23, 2024

Conversation

olynch
Copy link
Collaborator

@olynch olynch commented Dec 22, 2023

No description provided.

@olynch olynch marked this pull request as ready for review January 5, 2024 23:14
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7e3e833) 90.64% compared to head (261663d) 90.75%.

Files Patch % Lines
src/intertypes/json.jl 91.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   90.64%   90.75%   +0.11%     
==========================================
  Files          22       22              
  Lines        2074     2110      +36     
==========================================
+ Hits         1880     1915      +35     
- Misses        194      195       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olynch olynch changed the title Optional and Object WIP Optional and Object Jan 10, 2024
@olynch olynch requested a review from fivegrant January 10, 2024 21:23
Copy link
Collaborator

@fivegrant fivegrant left a comment

Choose a reason for hiding this comment

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

@olynch I'm still double checking a few things but I was wondering if you've noticed the JSON Schema validating on any JSON object. Would it break InterTypes to have the user specify which types get definitions? I think that might help a bit as to not stuff a bunch of definitions at the top level

src/intertypes/InterTypes.jl Show resolved Hide resolved
)
OptionalType(elemtype) => begin
schema = tojsonschema(elemtype)
schema[:type] = [schema[:type], "null"]
Copy link
Member

Choose a reason for hiding this comment

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

The schema of a type can be a list including the string null? Is this a JSONSchema idiom?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so? This is what showed up on the jsonschema website when I poked around for it, but I'm no jsonschema expert.

Copy link
Member

Choose a reason for hiding this comment

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

@fivegrant can probably confirm that it is what he would expect for a nullable field.

@olynch
Copy link
Collaborator Author

olynch commented Jan 10, 2024

@olynch I'm still double checking a few things but I was wondering if you've noticed the JSON Schema validating on any JSON object. Would it break InterTypes to have the user specify which types get definitions? I think that might help a bit as to not stuff a bunch of definitions at the top level

I'm not really sure what you mean by this with regard to "which types get definitions"?

@fivegrant
Copy link
Collaborator

@olynch If you look at the Petrinet Schema, you'll notice it only validates on object. InterTypes, however, produces a bunch of $defs:

"$defs": {
    "Parameter": {
      "type": "object",
      "properties": {
        "type": {
          "type": "string",
          "$comment": "Str"
        },
        "value": {
          "type": "number",
          "$comment": "F64"
        }
      },
      "required": [
        "type",
        "value"
      ]
    },
    "Condition": {
      "type": "object",
      "properties": {
        "type": {
          "type": "string",
          "$comment": "Str"
        },
        "value": {
          "type": "number",
          "$comment": "F64"
        },
        "domain_mesh": {
          "type": "string",
    ....
    "Thing": {
      "type": "object",
      "properties": {
        "a": {"type": "Condition"}, "b":{"type":"Parameter"}}
      }
    }

When validating against this schema with a bunch of $defs, many tools don't let you specify a particular one and see if a given JSON matches just one of the entries. For example, I just have to match one of the above to pass

@olynch
Copy link
Collaborator Author

olynch commented Jan 10, 2024

@olynch If you look at the Petrinet Schema, you'll notice it only validates on object. InterTypes, however, produces a bunch of $defs:

"$defs": {
    "Parameter": {
      "type": "object",
      "properties": {
        "type": {
          "type": "string",
          "$comment": "Str"
        },
        "value": {
          "type": "number",
          "$comment": "F64"
        }
      },
      "required": [
        "type",
        "value"
      ]
    },
    "Condition": {
      "type": "object",
      "properties": {
        "type": {
          "type": "string",
          "$comment": "Str"
        },
        "value": {
          "type": "number",
          "$comment": "F64"
        },
        "domain_mesh": {
          "type": "string",
    ....
    "Thing": {
      "type": "object",
      "properties": {
        "a": {"type": "Condition"}, "b":{"type":"Parameter"}}
      }
    }

When validating against this schema with a bunch of $defs, many tools don't let you specify a particular one and see if a given JSON matches just one of the entries. For example, I just have to match one of the above to pass

This is a valid point, but not really relevant to this PR. Can you open an issue for it?

But also, I would argue that this is a better way of doing it, because it maps closer to how one declares types in the programming languages that use intertypes.

Copy link
Collaborator

@fivegrant fivegrant left a comment

Choose a reason for hiding this comment

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

@olynch I brought the $defs issue because I think adding objects made the JSON Schema noticeably more permissive but it's been a minute since I've validated previously

Anyway, I waited to review this PR until I made sure everything worked in the MR one and it does so LGTM!

@fivegrant
Copy link
Collaborator

fivegrant commented Jan 11, 2024

small note: I added InterTypes so they match the examples exactly. Changes that require modifying the examples should be done in a different PR. ignore this. commented on wrong pr

@olynch olynch merged commit 3e93d2e into main Jan 23, 2024
9 checks passed
@epatters epatters changed the title Optional and Object Optional and Object for InterTypes Feb 15, 2024
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.

None yet

3 participants