-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[merge-conflicts] fix 'merge-conflicts' command category #6790
Conversation
- fixed the typo present in the command category prefix. - made the command prefix exportable so it can be resued by others. Signed-off-by: Vincent Fugnitto <[email protected]>
@@ -35,20 +35,20 @@ export interface MergeConflictCommandArgument { | |||
} | |||
|
|||
export namespace MergeConflictsCommands { | |||
const MERGE_CONFLIC_PREFIX = 'Merge Conflict'; | |||
export const MERGE_CONFLICT_PREFIX = 'Merge Conflict'; |
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 do we need to export this one?
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 included the export
so that others (extenders) would be able to access the prefix and be able to categorize their commands with existing ones if they wanted to. Do you believe we should not export?
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.
ah ok maybe you would have another PR where MERGE_CONFLICT_PREFIX
is referenced ?
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.
Exactly, let's say I'm an extender who wishes to use the prefix because they are contributing commands to the same category. Before the pull request, they'd have to duplicate and hard code the value while if it is exported they can simply reference 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.
Thank you Vincent!
Thank you @elaihau! |
What it does
How to test
@theia/merge-conflicts
(build & tests)Review checklist
Reminder for reviewers
Signed-off-by: Vincent Fugnitto [email protected]