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

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

Merged
merged 19 commits into from
Nov 1, 2023

Conversation

grzesiek2010
Copy link
Contributor

@grzesiek2010 grzesiek2010 commented Sep 21, 2023

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 XLSForm/xlsform.github.io#247

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests
  • run nosetests and verified all tests pass
  • run black pyxform tests to format code
  • verified that any code or assets from external sources are properly credited in comments

@grzesiek2010 grzesiek2010 force-pushed the PYXFORM-651 branch 3 times, most recently from e71f095 to 5469ae6 Compare September 21, 2023 22:35
pyxform/xls2json.py Outdated Show resolved Hide resolved
@grzesiek2010
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@grzesiek2010
Copy link
Contributor Author

grzesiek2010 commented Oct 25, 2023

@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.

pyxform/xls2json.py Show resolved Hide resolved
pyxform/validators/pyxform/android_package_name.py Outdated Show resolved Hide resolved
tests/test_image_app_parameter.py Outdated Show resolved Hide resolved
pyxform/validators/pyxform/android_package_name.py Outdated Show resolved Hide resolved
@lindsay-stevens
Copy link
Contributor

@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
Copy link
Contributor

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

@lognaturel
Copy link
Contributor

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

@lindsay-stevens lindsay-stevens merged commit aeedfca into XLSForm:master Nov 1, 2023
10 checks passed
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.

Add support for specifying a custom camera app for image questions
3 participants