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

Pass through itemset ref case #644

Merged
merged 2 commits into from
May 12, 2023

Conversation

lognaturel
Copy link
Contributor

@lognaturel lognaturel commented May 10, 2023

Closes #642

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

I decided to go with an exclude list for case normalization when first parsing parameters. The alternative I considered was to move case normalization to when parameter values are used by pyxform. I don't feel strongly between the two. Currently label and value are the only parameters that get set to user-provided strings. All of the others are numeric or enum values.

I also considered making sure all clients use label and value case-insensitively and leaving pyxform as it is. Even if we do choose to go that route, I don't think pyxform should be responsible for case normalization.

What are the regression risks?

The only risk I can see would be if some client somewhere counts on label and value references always being lowercase. However, alternate clients are not common.

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

No.

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

@lognaturel lognaturel force-pushed the case-sensitive-values branch from ae213bf to 22d885c Compare May 10, 2023 18:10
@lognaturel
Copy link
Contributor Author

@lindsay-stevens although this has been an issue for a while, now that we know about it I think it should be fixed as soon as we can. There's a Central release that's planned for next week and I'd love to include it.

Very interested in seeing what you think of the approach and whether you find it pragmatic or backwards.

@tiritea
Copy link

tiritea commented May 10, 2023

Not sure I entirely understand the full context, but...

The only risk I can see would be if some client somewhere counts on label and value references always being lowercase.

In my client I'm certainly not making assumptions that select values are supposed to be case-insensitive (eg always lowercase).

FWIW I dont see any mention of case (in)sensitivity in the spec: https://www.w3.org/TR/2003/REC-xforms-20031014/slice8.html#ui-common-choices-value

@lognaturel
Copy link
Contributor Author

select values

This is about the value and label references for defining itemsets. By default the value reference is name and the label reference is label. But those can be customized if e.g. you are using a file from another system that doesn't nave name and label elements/attributes/columns. In your XLSForm, you can specify something like value=XYZ_ID label=DisplayName. But currently the two values (XYZ_ID and DisplayName) are set to lowercase.

In an XML context it would be very unusual/go against the spec to treat element names as case-insensitive. I think it's would be a bit weird with CSV column names and geojson properties too though probably more common. Regardless, it's clear to me that pyxform should pass through the case provided by the user.

@tiritea
Copy link

tiritea commented May 11, 2023

Regardless, it's clear to me that pyxform should pass through the case provided by the user.

+1

Copy link
Contributor

@lindsay-stevens lindsay-stevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lognaturel looks good. It would be useful to add short comment in the code, or update the test description, to mention why these parameters should have preserved case - e.g. "to match the case in the external file".

@lindsay-stevens lindsay-stevens merged commit 7ecb4d5 into XLSForm:master May 12, 2023
@lognaturel lognaturel deleted the case-sensitive-values branch May 12, 2023 16:37
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.

Case is lost when specifying custom values for select_one_from_file label and value
3 participants