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

Configuration JSON schema #118

Merged
merged 7 commits into from Apr 21, 2020
Merged

Conversation

laudibert
Copy link
Contributor

Feature asked in #73.

Not sure if the schema should have "additionalProperties": false to help users realize when a field name was mistyped or if it's too constraining for when the schema might not be up to date with the code.

@laudibert
Copy link
Contributor Author

Also, I automatically filled the "description" of each field with whatever was on the README.md but I realized only afterwards that it might be a pain to maintain these pieces of documentation in both the README.md and the schema. Ideally though, the schema should have them though I guess.

README.md Outdated
@@ -412,6 +412,7 @@ Here's a list of available display field types:
* `url` - will render an anchor element with a clickable link
* `image` - will render an image from the url
* `colorbox` - will render a #RRGGBB hex string as an 80 x 20 pixel colored rectangle, overlaid with the hex color string
* `boolean` - will green or red dot
Copy link
Owner

Choose a reason for hiding this comment

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

  • will be

@dsternlicht
Copy link
Owner

Feature asked in #73.

Not sure if the schema should have "additionalProperties": false to help users realize when a field name was mistyped or if it's too constraining for when the schema might not be up to date with the code.

I think it's definitely should be part of the schema.

@dsternlicht
Copy link
Owner

Also, I automatically filled the "description" of each field with whatever was on the README.md but I realized only afterwards that it might be a pain to maintain these pieces of documentation in both the README.md and the schema. Ideally though, the schema should have them though I guess.

You did right. It's a bit pain, but very important.

@dsternlicht
Copy link
Owner

dsternlicht commented Apr 20, 2020

@laudibert That's an awesome PR for an extremely important feature! Kudos!
Let me know if it's ready to be merged.

@laudibert laudibert marked this pull request as draft April 20, 2020 12:08
Ludovic Audibert added 3 commits April 20, 2020 14:33
@laudibert laudibert marked this pull request as ready for review April 20, 2020 13:02
@laudibert
Copy link
Contributor Author

@dsternlicht, it should be ok now. The only problem that I encountered without any solution is that the "additionalProperties": false does not work with the keyword "allOf"that I used to manage inheritance for methods schema definitions. Therefore it is not set to false for the methods.

@dsternlicht
Copy link
Owner

I'm sorry to ask, but what's the meaning of "additionalProperties": false? It's not to allow other properties than the one you defined in the schema?

What's the "allOff" keyword?

@laudibert
Copy link
Contributor Author

I'm sorry to ask, but what's the meaning of "additionalProperties": false? It's not to allow other properties than the one you defined in the schema?

Yes but it works only one level at a time. Therefore it has to be defines for any object throughout the configuration. Not just at the root.

What's the "allOff" keyword?

It allows to combine schemas for one schema definition. See here for a good explanation. In the RESTool case, it allowed me to define the common properties of all methods in one place and then combine it with the specific of each method.

@dsternlicht
Copy link
Owner

dsternlicht commented Apr 20, 2020

Got it. Does schemas support inheritance from another file as well?

@laudibert
Copy link
Contributor Author

Yes, definitions can come from other files all loaded at the start of the app. I did not choose to do this though as I thought it might not be practical for people that are asking for the full schema.

I could change it though.

@dsternlicht
Copy link
Owner

@laudibert no need, just ask for my own general knowledge :)

@dsternlicht dsternlicht merged commit eea00a1 into dsternlicht:master Apr 21, 2020
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

2 participants