-
Notifications
You must be signed in to change notification settings - Fork 6
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
simplify logic on no arg commands #1584
base: develop
Are you sure you want to change the base?
Conversation
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.
Just a minor change.
I also pushed a fix to getFromAndTo
so it doesn't return Number.MAX_VALUE or Number.MIN_VALUE
which isn't supported by codemirror
}; | ||
} | ||
if ((argsNode && argsNode.length < expectedArgSize) || (!argsNode && expectedArgSize > 0)) { | ||
const { from, to } = getFromAndTo(argsNode ?? [stemNode]); |
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.
Change this to
const { from, to } = getFromAndTo(argsNode?.length ? argsNode : [stemNode]);
That way it will highlight the command stem instead of the TimeTag
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.
here is the commit for the code pushed:
* Was returning a min or max that was outside the scope of the editor. * Update custom folder after addressing this issue.
}; | ||
} | ||
if ((argsNode && argsNode.length < expectedArgSize) || (!argsNode && expectedArgSize > 0)) { | ||
const { from, to } = getFromAndTo(argsNode ?? [stemNode]); | ||
const pluralS = expectedArgSize - (argsNode?.length ?? 0) > 1 ? 's' : ''; |
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.
We have a pluralize
utility function in utilities/text.ts
if you'd like to use that instead.
I am not clear on the rationale for making a distinction on whether or not an arguments node is present, I don't expect the user to know and what is important is that the argument counts match. Let me know if I'm missing some nuance here.