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 rows to parameters column for text type #667

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

grzesiek2010
Copy link
Contributor

@grzesiek2010 grzesiek2010 commented Nov 21, 2023

Closes #616

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

The solution is consistent with how we handle other parameters in other question types. In My previous pull request, I added a new parameter for image questions (see #659) and this one solves the problem in the same way. I haven't considered any other approaches. I know there is a plan to Create a framework for adding parameters and then the solution would be different but for now we should follow the existing patterns.

What are the regression risks?

This is not a risky change. It's isolated and touches only reading parameters used in text 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#254

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-616 branch 2 times, most recently from 7aaacf5 to d3e6917 Compare November 21, 2023 02:12
pyxform/xls2json.py Show resolved Hide resolved
pyxform/xls2json.py Outdated Show resolved Hide resolved
tests/test_rows.py Outdated Show resolved Hide resolved
tests/test_rows.py Outdated Show resolved Hide resolved
@lindsay-stevens
Copy link
Contributor

Nothing to discuss here.

I mean, OK, but that's a bit flippant. You could say whether or not you followed the existing patterns for parameters (and why), whether or not you searched for other issues that might overlap with the linked issue, whether or not you tested the XForm output with Collect and/or Enketo, etc.

I often use PR descriptions, issues, and commit messages to do code archaeology to understand why things are a certain way, so adding some detail is worth the effort even if it seems obvious now.

@grzesiek2010
Copy link
Contributor Author

I mean, OK, but that's a bit flippant. You could say whether or not you followed the existing patterns for parameters (and why), whether or not you searched for other issues that might overlap with the linked issue, whether or not you tested the XForm output with Collect and/or Enketo, etc.

I often use PR descriptions, issues, and commit messages to do code archaeology to understand why things are a certain way, so adding some detail is worth the effort even if it seems obvious now.

Yeah, that makes sense. Even if something is obvious adding a short description might be valuable for other devs.

@lindsay-stevens lindsay-stevens merged commit acbfc43 into XLSForm:master Nov 24, 2023
10 checks passed
@lindsay-stevens
Copy link
Contributor

@grzesiek2010 thanks!

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 rows to parameters column for text type
2 participants