Skip to content
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 remaining args #82

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

MangelMaxime
Copy link
Contributor

I don't know how to make this change testable because it happens at the creation of the Pipeline so I added a demo script to showcase what it does and as an easy way to check that all is good for you.

One idea for making it testable was to change:

type PipelineContext with
static member Create(name: string) =

to

PipelineContext with 
  
     static member Create(name: string, ?forceArgs : string array) =

but I don't think this is good idea to change an API just to make it testable. Because I don't think we ever want to override the Environment.GetCommandLineArgs() in a normal usage.

member ctx.GetRemainingsCmdArgs() =
match ctx.ParentContext with
| ValueSome(StageParent.Pipeline p) -> p.RemainingCmdArgs
| ValueSome(StageParent.Stage s) -> s.GetRemainingsCmdArgs()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "s" after Remaining necessary? My English is not well to know that. But I see in the PipelineContext you are not using s for RemainingCmdArgs, so just wandering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

I checked and Spectre.Console.Cli use Remaining for the name of property.

One less letter to type is always good 😊

@@ -499,6 +499,23 @@ module StageContextExtensions =
| ValueSome(StageParent.Stage s) -> s.GetAllCmdArgs()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some comments on the GetAllCmdArgs to identify it will not return the remaing ones?

@albertwoo
Copy link
Contributor

@MangelMaxime for the unit test, I think you can write code like:

let mutable actualAllCmdArgs = []
let mutable actualRemainingArgs = []

pipeline "demo" {
    cmdArgs ["-p"; "demo"; "test1"; "v1"; "--"; "test2"; "v2"]
    stage "shows arguments" {
        run (fun ctx ->
            actualAllCmdArgs  <- ctx.GetAllCmdArgs()
            actualRemainingArgs  <- ctx.GetRemainingsCmdArgs()
        )
    }
    runImmediate
}

// Assert accordingly

@MangelMaxime
Copy link
Contributor Author

Regarding the unit test, originally I wrote something like you suggested but in the middle of writing this PR I changed how I implemented RemainingArgs and removed the tests because I think cmdArgs take effect after PipelineContext.Create has been called.

I will give it a try and check what happens.

@MangelMaxime
Copy link
Contributor Author

I checked and with my current implementation I can't test it the way you proposed.

However, looking at the comment in the code, I think I should also add the Args/Remaining split logic at this place too no?

/// Reset command line args.
/// By default, it will use Environment.GetCommandLineArgs()
[<CustomOperation("cmdArgs")>]
member inline _.cmdArgs([<InlineIfLambda>] build: BuildPipeline, args: string list) =
BuildPipeline(fun ctx ->
let ctx = build.Invoke ctx
{ ctx with CmdArgs = args }
)

@albertwoo
Copy link
Contributor

Yes, I think we should do that.

@MangelMaxime MangelMaxime force-pushed the feature/remaining_args branch from 65341c1 to cb963f6 Compare November 26, 2024 16:11
@MangelMaxime
Copy link
Contributor Author

@albertwoo This is ready for review.

By using making cmdArgs support Remaining args too and using an utility type to transform a list of args into a list of args + remaining args, I am able to indirectly test both places where this transformation takes place.

@MangelMaxime
Copy link
Contributor Author

The failing test doesn't seem related to my changes but caused by a slow CI runner.

@albertwoo
Copy link
Contributor

The failing test is related to a paralle tests, existing before, but did not figure out why 😂.

I will merge your PR, thanks for your contribution.

@albertwoo albertwoo merged commit b60533f into slaveOftime:master Nov 27, 2024
1 check passed
@MangelMaxime MangelMaxime deleted the feature/remaining_args branch November 27, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants