-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: Add support for overriding commands #51
Conversation
Allows overriding commands as long as they are defined in *different* Runfiles - Still can't define a command multiple times in *same* Runfile Command's list position based on first occurrence Command's display name based on first occurrence bug: Display Runfile in post-parsing error reporting chore: Capture Runfile in ast.Cmd and runfile.RunCmd chore: Add builtin flag to config.Command
@@ -169,47 +199,26 @@ Hello, world | |||
|
|||
###### Displaying Help | |||
|
|||
When displaying help text, run treats the command name as case-sensitive, displaying the command name as it is defined: | |||
When displaying help text, run displays command names as they are originally defined: | |||
|
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 way I'm reading this is that the command name as first defined w/r/t case will be in the list. I ASSUME that goes for the help text as well e.g. the help text of the first definition is what is displayed.
I'm hoping that I'm wrong. Seems to me that when the duplicate is encountered, its name case and its help text should be what is displayed in the list.
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 can't find this comment in the review section?)
Hey there! So, as coded the name case and list placement is taken from the first occurrence of the command, but everything else about the command, help text, doc-block args and variables, etc are all taken from the duplicate command.
|
||
INCLUDE Runfile-include | ||
|
||
## defined in Runfile |
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 would be a good place to redefine command1 with different help text to show how it redefines the command1 that was included.... or something like that.
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! Did you see the output of the list commands
example just below this? re:
command1 defined in Runfile-include
command2 defined in Runfile
The ## defined in *
is the help text in the example
ie. the include file does redefine command1
with different help text ...
Hey @MadBomber thanks for helping review this - I left a couple replies for you. |
It occurs to me that it might be meaningful to allow copying the original command's help text when a duplicate command doesn't provide any of its own. The original command is expected to be the most canonical, with duplicate commands likely being a bit more slap-dash, ie: Runfile
Then duplicates don't have to bother with duplicating the actual (and likely useful) help text.
The duplicate could get the default help text for free simply by not defining their own help text. I'll grind on this some more, but I think this will be a useful addition ... |
@MadBomber , so I did end up adding the feature to copy the first command's documentation (title + description) if an overriding command does not define their own. |
I believe you have accomplished your design goal. |
@MadBomber Thanks for the assist, merging now ! |
bug: Display Runfile in post-parsing error reporting
chore: Capture Runfile in ast.Cmd and runfile.RunCmd
chore: Add builtin flag to config.Command
cc: @MadBomber Care to give this a test drive / once-over ?