XLSForm / pyxform

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

Add support for specifying a custom camera app for image questions #659

Closed grzesiek2010 closed 7 months ago

grzesiek2010 commented 9 months ago

Closes #651

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

This is consistent with how we handle other parameters.

What are the regression risks?

This is not a risky change. It's isolated and touches only image questions.

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

Yes https://github.com/XLSForm/xlsform.github.io/issues/247

Before submitting this PR, please make sure you have:

grzesiek2010 commented 8 months ago

@lindsay-stevens I used nose2 instead of nose (as mentioned in the README) to run tests and everything was fine (the same in pyCharm). I did that because I had problems with using nose which is no longer maintained. It looks like the old nose does not recognize str | None: I'm I right?

lindsay-stevens commented 8 months ago

I recommend using the dev requirements as-is and using a supported Python version. In addition to the readme, the GitHub workflows show how to set up a dev environment from scratch.

The union operator for types was added in Python 3.10 (docs) as an alternative to Union[X, Y]. For the Union[str, None] case, it can (perhaps more clearly) be written as Optional[str, None]. Not related to the nose package.

grzesiek2010 commented 8 months ago

The union operator for types was added in Python 3.10 (docs) as an alternative to Union[X, Y]. For the Union[str, None] case, it can (perhaps more clearly) be written as Optional[str, None]. Not related to the nose package.

Right. It's fixed.

lindsay-stevens commented 8 months ago

@grzesiek2010 not sure if you're waiting on me but there are a few comments from the last review to address. Thanks

grzesiek2010 commented 8 months ago

@grzesiek2010 not sure if you're waiting on me but there are a few comments from the last review to address. Thanks

ups what exactly? I think I've addressed all of them. I've responded to all your comments with Done/Fixed and it's reflected in my commits.

lindsay-stevens commented 8 months ago

@grzesiek2010 I thought you could see the comments because you responded to the one about the Union operator. I have tagged you in the remaining ones. Thanks

lindsay-stevens commented 7 months ago

Thanks @grzesiek2010 ! @lognaturel I think the PR is ready to merge - do you want this in master or some other branch?

lognaturel commented 7 months ago

We’re going to master now! Can’t wait to get all the good stuff there released. 🚀