XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
77 stars 134 forks source link

Add initial support for entities #624

Closed lognaturel closed 1 year ago

lognaturel commented 1 year ago

Closes #622

Why is this the best possible solution? Were any other approaches considered?

This adds the desired support without increasing the size of workbook_to_json. It keeps the entities functionality relatively isolated which aligns well with the idea that this is an optional, experimental spec addition.

The entity node in the main instance has several attributes that each have bindings. I kept the JSON representation simple by using the existing parameters dictionary for the various aspects of entities that can be configured. Then I introduced a EntityDeclaration SurveyElement to create the attributes and their bindings. That feels better to me than trying to get an XForms-like representation in the json.

What are the regression risks?

I changed Survey to get the multiple bindings for a single form element. There used to be a xml_binding method for the current survey element's binding and xml_bindings for the bindings of the current element and descendants. I now have xml_bindings for the current survey element's possibly multiple bindings and xml_descendent_bindings for the bindings of the current element and descendants. I don't think there are regression risks but there's a chance this is being used as a public API. @ukanga @jnm any issues from your ends?

Does this change require updates to documentation? If so, please file an issue here and include the link below.

https://github.com/XLSForm/xlsform.github.io/issues/232

Before submitting this PR, please make sure you have:

ukanga commented 1 year ago

I changed Survey to get the multiple bindings for a single form element. There used to be a xml_binding method for the current survey element's binding and xml_bindings for the bindings of the current element and descendants. I now have xml_bindings for the current survey element's possibly multiple bindings and xml_descendent_bindings for the bindings of the current element and descendants. I don't think there are regression risks but there's a chance this is being used as a public API. @ukanga @jnm any issues from your ends?

From a quick check, we are not using it directly as a public API. More checks are needed on my end but nothing should hold this work back from our end.

lognaturel commented 1 year ago

Thanks, @ukanga! If you find there’s an issue, we can make changes, even if it’s after an initial release.

jnm commented 1 year ago

Thanks for the mention, @lognaturel. I don't foresee any problems.

lindsay-stevens commented 1 year ago

@lognaturel I know this is merged and released but just checking if you still want me to review this per your slack message?

lognaturel commented 1 year ago

Thanks, @lindsay-stevens! There are two things that I think would be helpful to get your feedback on:

High-level, I really tried not to make things messier and I want to confirm I achieved that. I'm also interested in whether you see any patterns we could/should reuse and/or any missed opportunities to lay useful groundwork.

lindsay-stevens commented 1 year ago

@lognaturel looks good to me. Some suggestions/notes