Drop XLS support#855
Open
lindsay-stevens wants to merge 50 commits into
Open
Conversation
- resolve new warnings on RUF059 (unused vars) and B017 (broad err). - re-checked commented out rules with ruff version markers, and the noted issues are still present, so updated the noted version.
- removes strenum copy which is only present in py3.11+ so that change is not backwards compatible. However, ruff warns that keeping it no good either since StrEnum exists in py311.
- refurb "A tool for refurbishing and modernizing Python codebases."
- the full pydocstyle ruleset has many changes so enabling some of them individually to keep the diffs manageable.
- the full pydocstyle ruleset has many changes so enabling some of them individually to keep the diffs manageable.
- the full pydocstyle ruleset has many changes so enabling some of them individually to keep the diffs manageable.
- the full pydocstyle ruleset has many changes so enabling some of them individually to keep the diffs manageable. - D415 is a bit strange when it comes to indented inline code blocks, so added backticks for those cases as well as URLs.
- the full pydocstyle ruleset has many changes so enabling some of them individually to keep the diffs manageable.
- the full pydocstyle ruleset has many changes so enabling some of them individually to keep the diffs manageable.
- The previous few commits selectively enabled the pydocstyle rules that resulted in changes that seemed reasonable to accept now. - The rules now in the ignore list are ones that would require a lot of manual rewrites (most of them have dozens of hits). - So this rearrangement enables other potentially useful pydocstyle rules that aren't currently triggering changes or warnings.
- `flynt is a command line tool to automatically convert a project's Python code from old "%-formatted" and .format(...) strings into Python 3.6+'s "f-strings".`
- conversion result in next commit so that file history is kept.
- used `pandoc --wrap=preserve -f rst -t gfm README.rst -o README.md` - markdown easier to remember and maintain - fixed grammatical error L112 "in a easily navigable format" - removed badges since this info is available on the same page as the readme (either GitHub or PyPI) and so one less thing to update.
- most of the files already are not xls anyway. - if they are being renamed already then it is a good time to move them into the fixtures directory so that their role is clearer.
- the form had a sheet named "Choices" (capitalised) but that is now a supported use case per test_xls2json.py
- the deleted case is essentially a valid survey header row with a
blank line before a single question. That is a pattern repeated in
test_xlsx_with_many_empty_rows, which has a few blank rows among the
survey questions (which seem to be intended as visual separators).
- thought about ways to refactor the old test anyway:
- can't do it with md_table_to_workbook because the markdown
pre-processing strips out empty rows, and re-inserting a blank
row after that seemed a bit fragile.
- using a dict passed to get_xlsform, but then trying a convert()
with that fails because the headers are taken from the first row.
- ultimately the old test seemed to be more about checking that
conversion still works in general for that blank row case (rather
than asserting anything specific about the processed form), so
the test_xlsx_with_many_empty_rows seemed like a complete repro.
- no other question types use the pattern "test_typed_[name]", but rather just use the type name e.g. "test_audit", "test_area", etc.
- seems to be different from example_forms/extra_columns in that the file modified here has data at the 20k-th row and 256th column, whereas the other file is a trickier case where empty columns exist in the file (as seen in the zipped XML) but no content after the apparent survey data (one question only).
- scenario same as test_group.py test_group__no_end_error__no_name
- The deleted file had a single question where the "bind: relevant"
header column had a single "LF" character. The "bind: relevant"
header is an alternative spelling of just "relevant".
- The replacement tests document existing behaviour in two ways:
- md data processed via xls2json_backends removes the empty col.
- dict data passed straight to xls2json just strips the LF.
- pyxform does not warn or error, Validate throws an error.
- The deleted file had three test cases, with existing coverage:
- choices header with spaces: test_sheet_columns.py
test_choice_filter_columns_not_normalised (case "e f")
- missing choices header: test_sheet_columns.py
test_process_row__bad_header_info* tests.
- row values with spaces: test_whitespace.py
- The deleted file had a single question, laid out as shown in the new markdown file (but in a more wordy way). - The xpath assertion checks that the "_comment" row doesn't end up as an attribute in the question binding, which is the most likely place if were to be processed.
- there does not seem to be a meaningful difference between the purpose and structure of this test and those in j2x_instantiation
- the form had calculate scenarios covered by test_calculate.py:
- a static calculation `2+2`
- a dynamic calculation of the form `if(${q1}=${q2},'y','n')`
- note items with label that displayed each calculate value
- the form had an interesting approach to implementing a cascading select via calculate rather than choice_filter, so that was translated into a new markdown test case.
- There are no references to these symbols in the code or tests.
- used in retained case insensitivity test that checks other formats.
- looking at the original commit that added this file, it's not entirely
clear why this test was added, but certainly it should be allowed for
users to have a choice named "type" so the test case is retained.
- deleted file had:
- a select_one where one of the names was "type": now covered in
test_choices_sheet.py test_choice_name_is_type__ok
- a select_one_external where one of the names was "type": now covered
in test_external_instances_for_selects.py
test_choice_name_is_type__ok
- the assertion in test_xls2xform.py that an external_choices form
produces itemsets.csv is covered by
test_external_instances_for_selects.py
test_itemset_csv_generated_from_external_choices
- The deleted file had the same data as the new test, just with longer names. The old test was quite specific about the question and choice data so these assertions are included in the new test. - Other tests in test_builder.py continue to assert the internal pyxform dict structure that the old test was checking for.
- The deleted file had a single 'time' question with a static default value. The handling of static defaults is covered by test_static_defaults.py but that test used the old string comparison assertions, so it was updated to use xpath. - The handling of time default values specifically is covered by test_dynamic_default.py TestDynamicDefaultSimpleInput, which e.g. checks that "01:02:55" is not a dynamic default, which is the same format pattern as the deleted test file "09:30:00".
- The deleted test was misleading in that the name of the test and the file suggested that it was about the "unknown" question type in the XLS file. But the test assertion is catching an error from the use of "bind:relevant" alongside the alias "relevant". - Unknown question types are covered by test_xlsform_spec.py test_warnings__unknown_control_group__with_name - Conflicting alias headers are covered by test_sheet_columns.py test_conflicting_aliased_values_raises_error
lindsay-stevens
force-pushed
the
remove-xls
branch
from
July 15, 2026 15:53
6baaa4d to
3e012d1
Compare
- The deleted file set a (seemingly invalid) style "ltr" and the
replaced tests checked that this value appears in the body as "class"
when a "style" is set, and no "class" is emitted otherwise. These two
test cases are now in test_settings.py
- There currently is no conversion or validation for the style value,
it is passed through as-is. This may or may not be intended but a
test is added to document this behaviour.
- The deleted files seem to be checking that a question label like
`${a} < ${b}` has the `<` escaped to `<` in the output. This case
and other escaping scenarios is covered by the test_xml.py
MinidomTextWriterMonkeyPatchTest cases.
- The patterns for how pyxform/variable references in labels are
emitted are covered by test_whitespace.py
test_whitespace_output_permutations. Since that test already is
running through a set of variations, a case that matches the
deleted file is added to avoid regressions.
- The deleted file had a single question of the type "hidden". The new
test checks that the output for this question type matches the
defaults from question_type_dict, which seems more useful than the
old test checking only the internal dict representation.
- There did not seem to be anywhere obvious to put this new test other
than a new module, and many of the existing test modules are about
validation and output of question types, so a new package is created
to collect those tests.
- The deleted files had a question of the type "include", and the "include.xls" fixture had an additional text question. The test usages were not using the create_survey_from_path parameter "include_directory" which activates the "include" directive behaviour. As a result, the tests were mainly on serialisation equivalence of: - for "include.xls", a "text" question plus an unknown question type - for "include_json.xls", an unknown question type. These scenarios are covered by the remaining alternative files. - Since `create_survey_from_path` is exposed as public in the top level __init__.py, tests were added for the "include_directory" parameter. There were no existing tests for this feature. A small edit to file_utils.py was needed to load files other than .xls and .json.
- The deleted file had a repeat group containing some items such as
select_one, select_multiple, integer, etc., and showed using pyxform
variable syntax, constraints, and "relevant" expressions. There was
also a "tutorial notes" column which explained these features.
- The demonstrated features are covered by test_fields.py and
test_repeat.py (and some other places as well)
- The documentation role of this fixture is covered by xlsform.org,
the template XLSForm reference, docs.getodk.org, and the forum.
- The deleted files had a "select_one or_other" question with a choices
list. There existing coverage for or_other in test_translations.py,
and new cases were added in test_select_or_other.py to document the
current behaviour with all select types.
- there are some expectedFailure tests which seem to be bugs.
- to make the new tests less repetitive or fragile, the select dict
in aliases.py was split into collections by type.
- The test in test_file_utils.py that was using specify_other was not
specific to that fixture, so new tests for the function
convert_file_to_csv_string were added to test_xls2json_backends.py.
- there are some expectedFailure tests which seem to be bugs.
- in preparation for removing xls support, the function was modified
to default to "xlsx" for non-csv input.
- Other serialisation tests in test_dump_and_load.py and
test_xform2json.py are still covered by other fixtures.
- The deleted file is not the same as the adjacent file "gps.xls": the xls has a question label "GPS", and no "choices" sheet. Both have a single "gps" type question named "location". - There is coverage of gps-related types in test_geo.py.
- The deleted file had a single "gps" type question named "location", and a non-localised label "GPS". There is coverage of this scenario and other gps-related types in test_geo.py. The serialisation tests this fixture was used in are covered by other fixtures.
- The deleted file was similar to the test case in test_group.py
test_table_list_appearance except that there were two such groups
with a table-list appearance.
- The existing test test_table_list_appearance is updated to use
exhaustive xpath checks to cover the previous XML segment check.
- An extra test is added to check the code path for preserving other
appearance values - the previous usage of "minimal" was misleading
since that is an appearance for selects so it does not appear in the
result xform.
- The deleted file was similar to the new test case in test_sms.py
test_sms_info except that there were some additional items in the
first group "a" and also a "meta" group with instance metadata (like
phonenumber, imei).
- As noted in the new test, there does not seem to be any special
handling of any sms* fields in relation to the xform output. So the
deleted test in test_builder.py was asserting that the sms* fields
are read and stored internally, but for no apparent reason.
- The new test at least demonstrates that old forms which may have
these items will still convert without error - though whether they
would still work as intended is unclear.
- The deleted file has the structure shown in test_loop.py test_survey_to_json_output (simple_loop), so the design is still in a test. It is nearly identical to the loop.xlsx fixture, in that it has a single integer item inside a loop directive, but the loop.xlsx feature also has a select item outside the loop.
- These files seem potentially useful to keep in some way:
- widgets is a relatively large form with many different control
and data types, appearances, and features.
- repeat_date_test is less clearly useful except for an interesting
serialisation scenario: a note label has a formatted date visible as
"12/12/2012" in the spreadsheet but which is output as
"2012-12-12 00:00:00" in the xform.
- each of the deleted xls files already had a xlsx equivalent, and the tests they were used in generally do not assume a file type. - retaining a small collection of files for serialisation tests seems useful for now to minimise the risk of regressions (vs deleting all).
- no remaining functions or tests using this - xlrd has not been maintained much for about 5 years now - the public converter "xlsform online" (currently at https://getodk.org/xlsform/) very minor XLS usage - current spreadsheet programs and tools can convert to XLSX
lindsay-stevens
marked this pull request as ready for review
July 17, 2026 17:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes https://github.com/getodk/security/issues/139
Why is this the best possible solution? Were any other approaches considered?
Branch based on deps-update / PR 853 branch so only commit b5ba83d (or later) are relevant.
Current approach:
Initially I started with the last step, but realised it was going to cause a huge diff or multiple commits with failing tests. Also considered just converting the XLS files to XLSX, but I figured that if the files are being touched anyway then it's probably the most relevant time to consider whether that test is still needed or has to use a file (as opposed to the current markdown test style with PyxformTestCase).
What are the regression risks?
Tools that support or expect XLS files would not work any more.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
Probably should mention it somewhere, at least on the Central upload page.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code