A bunch of feature

[Arthur-Milchior]

Hi,

First, thanks for your project. It is really nice. I intend to use it in a software I contribute to: anki.
Here are a lot of change which I believe are non controversial. The details are given in each commit.
There are some comments. USAGE.md contains explanation about a lot of option.
Default value for number, int, string, boolean. Default respect minimal size of list and string.
String which gets too long are trimmed.

I don't really understand the point of ui_schema. It seems strange. It means that if the same property name is used twice, it must correspond to the same ui. And that it can be applied only on object's element, not on array's element.

Can you please tell me whether you are interested in working together, in which case I'd be happy to do more PR as I had more feature. Or are you not interested anymore (after all, there have been no change since 2018), in which case I'll fork it and do my change on my side.

[agoose77]

I suggest changing this to return defaults.get(schema_type)

[Arthur-Milchior]

I don't understand your point.
your code return a function. It does not apply it to the schema.
Of course I could do
return defaults.get(schema_type, lambda _: None)(schema)
but it does not really seems interesting

[agoose77]

Hi @Arthur-Milchior, thanks for the big contribution!

I'd be happy to collaborate, but I don't really have much time at the moment to work on this. It would be nice to merge any changes into master, though, so that others could use benefit from them.

The UI schema is designed to add UI meta-data to the schema, e.g. specifying the kind of UI widget to display when the schema is degenerate (e.g. strings can be filepaths, textarea, or textinput fields). This is currently only used for this particular use case, see

qt-jsonschema-form/qt_jsonschema_form/form.py

Line 83 in 51f6787

widget_variant = ui_schema.get('ui:widget', default_variant)

[Arthur-Milchior]

I am not sure what you mean by "degenerate". Do you mean that, by itself, the schema does not have enough information to know how to present the result ?

Anyway, my question was quite different. I wanted to know why you didn't add "ui:widget" property into the schema itself ? As far as I can tell, it's perfectly valid to add extra properties to schemas. They are simply ignored by the validator. It seems to me that it helps to be able to have all informations together.
I do admit that having ui_schema is also nice, especially if you want to apply this library to a schema which already exists and that you can't change, so I actually suggest allowing "ui:widget" parameters to be both in the schema and in ui_schema.
Do you see any reason not to do it ? If I do this change, would you accept it ? 

[agoose77]

Yes, there are some fields for which there are multiple valid representations. The additional "schema" (though really metadata) specifies the UI representation.

I prefer to follow the approach of the React Schema Form implementation, as it decouples the UI layout from the data schema. This is both better separation of concerns, but more importantly permits us to use existing schemas which do not include this meta-data, without having to monkey patch them. Therefore, I would prefer to keep the two schemas separate.

[Arthur-Milchior]

My point was to still allow UI layout to be in a separate file. But also to add it potentially in the schema. So that user can decide which way is better for them.
In my particular case, my users won't use standard schema and will need to write their own. So asking them to put UI indication at the same time is not a problem. And actually, it allows to have all informations at the same place, which is actually easy to read and debug.
If you would reject it, I'll simply fork the library for my own purpose. I'll keep a branch with all changes I suspect you may accept and keep this feature in a separate branch.

By the way, is there any reason you didn't merge this PR ? Not enough time to review it in details ?

[agoose77]

I think that I would prefer to keep the UI schema separate. If you'd still be happy to maintain a fork, let's add a method get_widget_variant to the WidgetBuilder class, which you can then override, e.g

def get_widget_variant(self, schema_type: str, schema: Dict[str, Any], ui_schema: Dict[str, Any]) -> str:
    try:
        default_variant = self.widget_variant_modifiers[schema_type](schema)
    except KeyError:
        default_variant = self.default_widget_variants[schema_type]

    if "enum" in schema:
        default_variant = "enum"
           
    return ui_schema.get('ui:widget', default_variant)

In your subclass you would just modify this method

def get_widget_variant(self, schema_type, schema, ui_schema):
    return schema.get('ui:widget', super().get_widget_variant(schema_type, schema, ui_schema))

I haven't merged the PR yet as I'm currently moving house. I'll take a look when I get some time :) Thanks for the effort!

[Arthur-Milchior]

get_widget_variant seems an excellent idea. Better than what I did actually !
You're welcome for the effort. I expect to have at least thousands of users, potentially a million if I can get my PR accepted in Anki. And, I hope this won't be offensive, but I need to add more feature before it can be actually used easily by anyone.
E.g. I still need to accept properties which are not in the schema

Good luck with moving

[Arthur-Milchior]

The method has been added as you wanted, at least if I understood correctly

[agoose77]

This is great work, thanks! Would you be able to add some tests for the new features? e.g. test that get_widget_variant behaves as expected?

[Arthur-Milchior]

Where do you want the tests to be ?

[agoose77]

Let's create a tests directory and use pytest?

[agoose77]

I'm sorry that I'm still not able to review this. A few things I've thought about:

• As we support more of the JSON Schema spec, we need to version according to the schema version. This might mean specifying an enum string for the spec version.
• Some of the properties you implement e.g. minContains I cannot find in any existing specs. I might not be looking hard enough, but are they valid?

[Arthur-Milchior]

minContains seems to be in the current json schema validation draft. https://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.6.4.5 or https://datatracker.ietf.org/doc/draft-handrews-json-schema-validation/?include_text=1
As far as I'm concerned, I'd be happy to let the validator deal with the schema version and not care about it in the GUI.
I do not know of any change between schema 4 and 7 which would make the form different anyway.