-
Notifications
You must be signed in to change notification settings - Fork 588
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
Add function to add additional command-line arguments in Fake.DotNet.DotNet.Options as a sequence of strings. #2044
Add function to add additional command-line arguments in Fake.DotNet.DotNet.Options as a sequence of strings. #2044
Conversation
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
@@ -498,6 +498,8 @@ module DotNet = | |||
lift (fun o -> { o with Verbosity = verb}) x | |||
let inline withCustomParams args x = | |||
lift (fun o -> { o with CustomParams = args}) x | |||
let inline withAdditionalArgs args x = | |||
withCustomParams (args |> Args.toLinuxShellCommandLine |> Some) x |
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.
Is Args.toLinuxShellCommandLine
the suitable option clear? The other alternative (toWindowsCommandLine
) was far too complicated to be considered.
But I thought that .NET Core uses thinks like --option
instead of /option
.
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 think you need to use toWindowsCommandLine as this is the one netcore uses
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.
Done.
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
@@ -498,6 +498,8 @@ module DotNet = | |||
lift (fun o -> { o with Verbosity = verb}) x | |||
let inline withCustomParams args x = | |||
lift (fun o -> { o with CustomParams = args}) x | |||
let inline withAdditionalArgs args x = | |||
withCustomParams (args |> Args.toWindowsCommandLine |> Some) x |
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 we use None
if the list is empty?
This is actually a nice idea. Only thing is we should probably add a comment explaining that this uses the other field (such that a user knows that using |
Wow you are fast |
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
@@ -498,6 +498,8 @@ module DotNet = | |||
lift (fun o -> { o with Verbosity = verb}) x | |||
let inline withCustomParams args x = | |||
lift (fun o -> { o with CustomParams = args}) x | |||
let inline withAdditionalArgsAsList args x = |
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.
It's a seq
, but I'm pretty sure they would get the point.
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.
What I meant was for withX
helpers it is clear they write to X
for the withAdditionalArgs
helper it is not really clear what it is doing (well it is clear but doesn't match the scheme). That's why I would either opt for something with withCustomParams*
prefix or a comment (yes it would stand out but the helper is a bit "special" so that would be fine imho) ;)
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.
Done.
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
@@ -498,6 +498,11 @@ module DotNet = | |||
lift (fun o -> { o with Verbosity = verb}) x | |||
let inline withCustomParams args x = | |||
lift (fun o -> { o with CustomParams = args}) x | |||
/// Sets custom command-line arguments expressed as a sequence of strings. | |||
/// This function overwrites and gets overwritten by `withCustomParams`. | |||
/// Furthermore, there is no way to get back the arguments as a sequence of strings. |
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.
there is no way to get back the arguments as a sequence of strings
Technically, you can with Args.fromWindowsCommandLine
, but fine by me ;)
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
@@ -498,6 +498,11 @@ module DotNet = | |||
lift (fun o -> { o with Verbosity = verb}) x | |||
let inline withCustomParams args x = | |||
lift (fun o -> { o with CustomParams = args}) x | |||
/// Sets custom command-line arguments expressed as a sequence of strings. | |||
/// This function overwrites and gets overwritten by `withCustomParams`. | |||
/// Furthermore, there is no way to get back the arguments as a sequence of strings. |
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.
there is no way to get back the arguments as a sequence of strings
Technically, you can with Args.fromWindowsCommandLine
, but fine by me ;)
Anyway good idea to do it like that, thanks! I didn't realize this is an option when closing the issue :) |
Merging once CI is green. |
I will delete the last comment line. I actually don’t think it’s needed.
Sent from Mail for Windows 10
From: Matthias Dittrich
Sent: Δευτέρα, 30 Ιουλίου 2018 20:20
To: fsharp/FAKE
Cc: Theodore Tsirpanis; Author
Subject: Re: [fsharp/FAKE] Add function to add additional command-linearguments in Fake.DotNet.DotNet.Options as a sequence of strings. (#2044)
@matthid commented on this pull request.
In src/app/Fake.DotNet.Cli/DotNet.fs:
@@ -498,6 +498,11 @@ module DotNet =
lift (fun o -> { o with Verbosity = verb}) x
let inline withCustomParams args x =
lift (fun o -> { o with CustomParams = args}) x
+ /// Sets custom command-line arguments expressed as a sequence of strings.
+ /// This function overwrites and gets overwritten by `withCustomParams`.
+ /// Furthermore, there is no way to get back the arguments as a sequence of strings.
there is no way to get back the arguments as a sequence of strings
Technically, you can with Args.fromWindowsCommandLine, but fine by me ;)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
…DotNet.Options as a sequence of strings.This funcion internally converts them to a single string.It does not break anything.The downside is that the arguments cannot be _read_ as a sequence of strings.
Why wait - I want to have this part of the release :P |
This funcion internally converts them to a single string.
It does not break anything.
The downside is that the arguments cannot be read as a sequence of strings.
This PR was made to address the hastily closed #2042.