-
Notifications
You must be signed in to change notification settings - Fork 144
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
New sub/person substitution algorithm. #208
Conversation
…tions when the number of subs is too large
src/brain.coffee
Outdated
|
||
# Convert the placeholders back in. | ||
# Take the original message with no punctuation | ||
pattern = msg.replace(/[.?,!;:]/, "") |
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 could probably reuse the @master.unicodePunctuation
regexp, and that way if UTF-8 supporting users change the set of punctuation symbols to fit their target language it will apply here too.
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.
done!
src/brain.coffee
Outdated
msg = msg.replace(new RegExp("\x00<#{cap}>\x00", "g"), result) | ||
# Look for words/phrases until there is no "spaces" in pattern | ||
while pattern.indexOf(" ") > -1 | ||
giveup++ |
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.
At time of writing, this line is breaking the unit tests because the variable name is tries
, not giveup
; so rename one of the variables to match.
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.
The variable name is even giveup, but it was undeclared
src/brain.coffee
Outdated
giveup++ | ||
# Give up if there are too many substitutions (for safety) | ||
if giveup >= 50 | ||
@warn "Too many loops in substitution placeholders!" |
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.
The warning message should more accurately reflect what it's doing. It looks like your new algorithm doesn't use the substitution placeholders like the old one did (which is good 😄), but warnings like this might say instead "Too many loops when handling substitutions!" or similar.
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.
done!
src/utils.coffee
Outdated
@@ -154,3 +154,6 @@ exports.isAPromise = (obj) -> | |||
typeof obj.then is 'function' and | |||
typeof obj.catch is 'function' and | |||
typeof obj.finally is 'function' | |||
|
|||
exports.nIndexOf = (string, match, index) -> | |||
return string.split(match, index).join(match).length |
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.
Can you add documentation to this function? I had to stare at it for a while to figure out what it does. Something like,
##
# int nIndexOf (string input, string delimiter, int wordCount)
#
# This function splits an input string and then takes the first `wordCount` words from it,
# rejoins them and returns the length of that part of the input.
#
# The return value is the index in the original input string where the first N words end.
##
The name of the function, nIndexOf
isn't very descriptive either, but I can't think of a better name for it.
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.
done
src/brain.coffee
Outdated
else | ||
# Otherwise Look for substitutions in a subpattern | ||
while subpattern.indexOf(" ") > -1 | ||
subgiveup++ |
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.
I didn't see where this variable is declared; this line would give an error if you don't initialize the variable first.
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.
done!
It Makes a lot of less interactions when the number of subs is too large