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

Add function to add additional command-line arguments in Fake.DotNet.DotNet.Options as a sequence of strings. #2044

Merged
merged 1 commit into from
Jul 30, 2018

Conversation

teo-tsirpanis
Copy link
Contributor

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.

@@ -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
Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -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
Copy link
Member

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?

@matthid
Copy link
Member

matthid commented Jul 30, 2018

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 withCustomParams and withAdditionalArgs doesn't work at the same time). Or we change the naming accordingly (withCustomParamsAsList for example?)

@matthid
Copy link
Member

matthid commented Jul 30, 2018

Wow you are fast

@teo-tsirpanis
Copy link
Contributor Author

I will do the second. Documentation in these files is already non-existent, and documenting just one function would stand out. 😂😂

Wow you are fast

A meme that elaborates on your claim of my speed of response

This is why it took me so much. 😛

@@ -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 =
Copy link
Contributor Author

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.

Copy link
Member

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) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -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.
Copy link
Member

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 ;)

@@ -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.
Copy link
Member

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 ;)

@matthid
Copy link
Member

matthid commented Jul 30, 2018

Anyway good idea to do it like that, thanks! I didn't realize this is an option when closing the issue :)

@matthid
Copy link
Member

matthid commented Jul 30, 2018

Merging once CI is green.

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Jul 30, 2018 via email

…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.
@matthid matthid merged commit 9b63d20 into fsprojects:release/next Jul 30, 2018
@matthid
Copy link
Member

matthid commented Jul 30, 2018

Why wait - I want to have this part of the release :P

@teo-tsirpanis teo-tsirpanis deleted the with-additional-args branch August 1, 2018 11:19
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.

Shouldn't Fake.DotNet.DotNet.Options.CustomParams be a list of strings?
2 participants