-
Notifications
You must be signed in to change notification settings - Fork 44
Text block comments #454
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
base: main
Are you sure you want to change the base?
Text block comments #454
Conversation
PR Summary
|
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.
Hey, a couple of required changes before I can approve this — let's keep the PR narrowly focused on just the specific issue of command triggers in code blocks
package.json
Outdated
@@ -59,7 +59,7 @@ | |||
"ts-node": "10.9.2", | |||
"tsx": "^4.19.2", | |||
"typescript": "^5.7.3", | |||
"vitest": "1.6.0" | |||
"vitest": "^1.6.1" |
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.
Why is this changing?
src/features/commands.ts
Outdated
while (i < position) { | ||
if (text.slice(i, i + 3) === "```") { | ||
insideTriple = !insideTriple; | ||
i += 3; | ||
} else if (text[i] === "`" && !insideTriple) { | ||
insideSingle = !insideSingle; | ||
i++; | ||
} else { | ||
i++; | ||
} | ||
} |
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'm a bit concerned about the way this code works — for a string of length n, it will create n substrings, which seems like a lot of CPU and memory. Since the last PR had issues with resource contention, this makes me nervous.
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.
Actually it's more resource intensive than I thought — it will create n substrings for every command trigger, and we have ~ 40 commands with ~ 50 triggers. So that means that a message with 150 characters will create 150 3-character substrings 50 times — I think this is a no-go the way it's written
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.
Hmm okay, I will think of a better approach for this
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 wonder, couldn't you use replaceAll
to remove all content in-between backticks ` (other forbidden letters). Then you could compare, if the trigger name is still there.
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.
shud be possible, will try it @kristersd . thanks !!
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.
In the meantime, check the latest commit @kristersd if tats an optimal approach
Seems like this could be made a lot simpler utilizing a slightly more complex regex:
this should match |
Can someone help me on why these 8 test cases fail?
|
okay I added this regex |
Added a new logic to check the commands inside nested / normal codeblocks