-
Notifications
You must be signed in to change notification settings - Fork 57
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
Added regex support #142
base: main
Are you sure you want to change the base?
Added regex support #142
Conversation
Added regex support for ignore strings, required strings and replacement of notification text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
In addition to the requested code changes, the descriptions for all 3 options should mention how to enable regex. For now we can assume the user knows or can figure out for themselves how to write regex, they just need to be made aware of how to enable it.
Strings:
require_ignore_strings_dialog_msg
tts_text_replace_dialog
"(?i)${Pattern.quote(pair.first)}".toRegex(), pair.second) | ||
val pattern = | ||
if (pair.first.startsWith(Constants.REGEX_PREFIX)) | ||
Regex(pair.first.removePrefix(Constants.REGEX_PREFIX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This crashes when a notification is received if the regex is malformed. It should catch PatternSyntaxException
as you did for require/ignore and I would suggest falling back to Pattern.quote
. I would also suggest, in all 3 options, validating the regex and showing an error (toast is fine) if it's malformed before allowing the user to save it.
I think we want (?i)
or RegexOption.IGNORE_CASE
here for consistency with the standard string and ignore/require regex, and since the description says it's case insensitive. Alternatively, update the description to indicate that the regex is case sensitive.
Regex(pattern, RegexOption.IGNORE_CASE) | ||
.containsMatchIn(ttsMsg) | ||
} catch (e: PatternSyntaxException) { | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.printStackTrace()
would be a good idea in case someone runs into problems and needs to debug it.
Tested pr, quite simple, that adds regex support for the ignore, require, and replace functionalities.
It works by adding
$regex:
to the start of each string, and will else function as usual.Might be worth adding some example patterns, maybe to the readme or something. Not a must.
An example pattern to insert the word space between numbers:
$regex:(?<=\d)\s+(?=\d)
replace withspace
An example pattern to insert spaces between numbers:
$regex:(?<=[0-9])(?=[0-9])
replace withit is also possible to use group references by just typing $1, $2 etc.