Gracefully handle regular expression errors in speech dictionaries#10421
Merged
Conversation
…ssing text for a given dictionary entry, log an error, remove the entry from memory, and continue processing further entries.
|
PR introduces Flake8 errors 😲 See test results for Failed build of commit d9c9cba4ef |
LeonarddeR
reviewed
Oct 25, 2019
| text = entry.sub(text) | ||
| except re.error as exc: | ||
| dictName = self.fileName or "temporary dictionary" | ||
| log.error(f"Invalid dictionary entry {index+1} in {dictName}: \"{entry.pattern}\", {exc}") |
Collaborator
There was a problem hiding this comment.
Suggested change
| log.error(f"Invalid dictionary entry {index+1} in {dictName}: \"{entry.pattern}\", {exc}") | |
| log.error(f"Invalid dictionary entry {index+1} in {dictName}: {entry.pattern!r}, {exc}") |
LeonarddeR
approved these changes
Oct 25, 2019
LeonarddeR
left a comment
Collaborator
There was a problem hiding this comment.
APart from the only comment I made about the error log, this looks fine!
Member
Author
|
@LeonarddeR: I'm not sure about printing the pattern as raw, as then the
regular expression becomes much harder to read as all back slashes are
escaped E.g. '(\\d)(?=\\d)'
|
Collaborator
|
A, hadn't thought about that one. In that case, go ahead.
|
Contributor
@michaelDCurran, seems PR #10172 is not the one you meant to reference here. |
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
5 participants
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.
Link to issue number:
Fixes #10334
Summary of the issue:
In Python 3, re.sub raises re.error if a numeric replacement group is specified yet there is no matching group in the pattern. Python 2, silently ignored this.
In the past, users have deliberetly or otherwise created entries that are invalid in this way, but in NVDA 2019.2.1 and lower were just silently ignored. Now, they break NVDA in a way such that no speech works while those entries exist.
Description of how this pull request fixes the issue:
NVDA will now catch re.error, log the error, remove the entry from meory, and continue on processing other entries.
Testing performed:
Tested with the default.dic provided in #10334. Plus created several other invalid and valid entries, both in the default dictionary and temporary dictionary.
The invalid entries were logged once and then no longer processed.
Known issues with pull request:
None.
Change log entry:
Bug fixes:
Invalid regular expressions in speech dictionaries no longer completely break speech in NVDA. (#10334)