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

Make forms inherited from a base form #1319

Merged
merged 4 commits into from
Jan 19, 2022
Merged

Conversation

egekorkan
Copy link
Contributor

Addresses #1311

Some basic tests are available at https://www.jsonschemavalidator.net/s/lNpA9CYs

Copy link
Contributor

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

LGTM!

Note: I recommend ignoring whitespaces since the PR touches lots of them...

Copy link
Contributor

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

forms for property should point to form_element_property

while the definition of form_element_property should have the "allOf" construct.

{
  "allOf": [{ "$ref": "#/definitions/form_element_base" }],
  "properties": {
     "op": {
        ...
     }
  }
}

Moreover, form_element_base might also need the op values.. in this case defined as string ... with no enumerations. Thinking of it in an hierarchical fashion root/property/actions/events should be more specific.

I "believe" doing so it might work..

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

I agree with @danielpeintner having allOf inside form_element_property it is more self-contained.

Regarding ts typings it may be beneficial to have a generic Form type:

{
  // schema ...
   form : {
      oneOf: [
           {  $ref : "#/definitions/form_element_property" },
           {  $ref : "#/definitions/form_element_action" },
           {  $ref : "#/definitions/form_element_event" },
           {  $ref : "#/definitions/form_element_root" },
       ]
   }
}

Although this would not have any validation purposes, it should also solve: eclipse-thingweb/node-wot#550

@egekorkan
Copy link
Contributor Author

Three points about the comments above:

  • Although this would not have any validation purposes : This is sadly correct and I would not support this direction since the schema would lose its main purpose. One possible way forward is to do some preprocessing before generating TS types 🙈
    - form_element_property should have the "allOf" construct. : I am not sure what is meant here but I have done this: https://www.jsonschemavalidator.net/s/Fy7VkyFE based on How to do inheritance? json-schema-org/json-schema-spec#348
  • Moreover, form_element_base might also need the op values.. in this case defined as string ... with no enumerations. Thinking of it in an hierarchical fashion root/property/actions/events should be more specific. I have fixed it at the link above This is what you mean right?

Some other comments:

  • What is needed is that a schema that validates and still generates TS types accordingly. Since the op value depends on the affordance it is placed in and that JSON Schema cannot "backtrack" to add requirements, I do not know how to really fix this.

@egekorkan
Copy link
Contributor Author

Call of 15.12.2021:
@egekorkan add the form definition proposed by @relu91 in the comment above but also adding $comment that this is only an unused definition that is used for generating ts types.

start fixing tm schema tests
@egekorkan egekorkan marked this pull request as draft December 30, 2021 00:12
@egekorkan
Copy link
Contributor Author

Converting to a draft since I have found some major issues with the tests. Basically none of the tests were working, now they do and reveal an issue with the regex.

const placeholderPattern = "^.*{{2}[ -~]+}{2}.*$"; seems to be not a valid regex. I get Invalid regular expression ... Lone quantifier brackets

I need to understand it properly before making it available

@egekorkan egekorkan marked this pull request as ready for review January 3, 2022 23:52
@egekorkan
Copy link
Contributor Author

I have learned that I have to encapsulate { into [}] to avoid regex errors. Ready to merge

@sebastiankb
Copy link
Contributor

from today's TD call, decided to merge

@sebastiankb sebastiankb merged commit d23e9e8 into main Jan 19, 2022
@egekorkan egekorkan deleted the ege-schema-form-structure branch January 28, 2022 22:32
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