-
Notifications
You must be signed in to change notification settings - Fork 149
747: update identifier validation message now that leading colons aren't allowed #795
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
Open
lindsay-stevens
wants to merge
33
commits into
XLSForm:master
Choose a base branch
from
lindsay-stevens:pyxform-747
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
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
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages - add test to verify error raised, since no other test seems to check for this error case or message.
- organising error messages
- organising error messages
- organising error messages
- the modified tests pass anyway but would have asserted that each letter in the parameter was in the error e.g. ["M", "i", "s", "s"] - added type check and tests for error/warning contains/not-contains
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages
- organising error messages - reworded error message to be consistent with the underlying XML "Name" rule/token, except that ODK form clients generally don't support having a colon for the first character, and not mentioning non-ascii unicode letters since that's probably too technical. - preceding code applies the additional entity dataset name rules for not allowing names starting with a period or two underscores.
- organising error messages - preceding code applies additional entity save_to name rules for not allowing reserved words or names starting two underscores.
- organising error messages - the replaced validation regex is pretty much the XML name rule and the error message sounds like that was the intent (although the value is put into an attribute like `<value ref="VAL"/>` not a tag).
- organising error messages
- organising error messages - in survey_element.py the code that shows the exact offending character seems to have been added for the purpose of checking the form_name, which corresponds to the Survey.name attribute, which is used as the tag name of the primary instance element. When uploading a XLSForm to an online converter this name defaults to "data", and it is only set to the basename the XLS/X file name when using the CLI, or some other value when using pyxform as a library. This code would not apply to most other names since workbook_to_json (or other) code checks names before survey_element.py sees them. Also the regex INVALID_XFORM_TAG_REGEXP is different to the rule in is_xml_tag. So the per-character check was removed since it seems unlikely to be useful and is inconsistent with other name validation.
- organising error messages
- organising error messages
- organising error messages
- not caught by relevant test since it only requires the substring "must also specify an image".
9065ec2 to
3e497c9
Compare
- it's possible to set the primary instance root node name via the setting "name" (which overrides other ways to set the name), so check the settings code path as well.
Contributor
Author
|
Realised there's a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #747
Why is this the best possible solution? Were any other approaches considered?
The majority of these commits are organising error/warning messages into the
errors.pymodule, which is what I had in mind for theErrorCodepattern which was added to pyxform a few months ago. After this PR about 30 error messages are organised there, but pyxform raises an error at about 170 locations and many of those have distinct messages.The goal of this stage of organisation is to try and identify all the errors/warnings which deal with identifiers, so that the validation approach and message content can be more consistent with each other and with the documentation. Also to take the opportunity to move over error/warning messages that are already using the
errors.Detailclass or seemed easy to move. There are still some name-related errors that have not been moved (e.g.Audits must always be named 'audit.', etc), but they don't specifically deal with identifier grammar. There is also still a name-like regex check for the "app" parameter but that requires an android package name which has different rules to XML names.What are the regression risks?
If library or script usages were looking for specific error strings then these changes would break them. But putting the errors into an enum/object should make it easier to avoid looking for specific strings - as is shown in the tests that use
ErrorCode.Does this change require updates to documentation? If so, please file an issue here and include the link below.
Maybe? #630 says ODK tools don't support having a colon as the first character in identifier names but that doesn't seem to be stated in the ODK XForms Specification. On one hand maybe it is not a hard requirement from a spec perspective, but on the other hand it is functionally required via pyxform validation and not being supported by ODK form clients.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code