-
Notifications
You must be signed in to change notification settings - Fork 368
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 Interactive mode with target selection #1437
base: develop
Are you sure you want to change the base?
feat: Add Interactive mode with target selection #1437
Conversation
@@ -19,7 +19,7 @@ public interface INukeBuild | |||
{ | |||
void ReportSummary(Configure<Dictionary<string, string>> configurator = null); | |||
|
|||
internal IReadOnlyCollection<ExecutableTarget> ExecutableTargets { get; } | |||
IReadOnlyCollection<ExecutableTarget> ExecutableTargets { get; } |
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.
Should be reverted
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.
From my side this change was intentional. To properly be able to override and customize OnNoTargetsSpecified()
developers need access to the general list of targets which are there as option.
e.g. in some projects we'd like to only be able to have a "single target" selection as this fits the project setup better and would like to change the default "multi selection" implementation shipped in nuke.
As alternative I could pass the executableTargets a parameter to OnNoTargetsSpecified
.
1131d5a
to
dc902f8
Compare
I wouldn't go for "automatically detect local terminal and execute the interactive mode when no parameters given". I would rather add an option on |
@ITaluone I agree and there are some considerations around that in the proposal. One important detail in this implementation is: there is an additional
|
df620ac
to
482f5ac
Compare
I confirm that the pull-request:
This PR contains a proposal how #1436 could be implemented. Structurally we should still discuss where what should go but it already gives a feeling on what a user gets with this change.
Current design decisions
InvokedTarget
loading logic into theExecutableTargetFactory
NukeBuild
to allow people overriding and customizing it.ExecutableTargets
public so custom implementations can use it.Terminal
class.--help
Any feedback is welcome to complete this change.
Demo
rider64_xcwuHlW2Dh.mp4