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

Added support for adding custom arguments to tools. #588

Merged
merged 1 commit into from
Dec 17, 2015
Merged

Added support for adding custom arguments to tools. #588

merged 1 commit into from
Dec 17, 2015

Conversation

patriksvensson
Copy link
Member

Closes #500

@patriksvensson patriksvensson changed the title Added support for adding custom arguments to tools. [WIP] Added support for adding custom arguments to tools. Dec 15, 2015
@gep13
Copy link
Member

gep13 commented Dec 16, 2015

@patriksvensson there seems to be something screwy going on here:

image

with the names of the files in this PR.

Any ideas?

@patriksvensson
Copy link
Member Author

@gep13 It's just a convention to name files containing generic classes using the CRef naming (i.e. T:TestNamespace.GenericClass```1). We hadn't had the need for it before since there are no classes with the same name that only differs by generic argument count. CoreFX for example uses the same conventions.

Can change it to something else if you want to, but I think we should have some convention for it.

@gep13
Copy link
Member

gep13 commented Dec 16, 2015

@patriksvensson Gotcha! No, no need to change it. I just hadn't seen this before. My initial thought was that something had gone wrong with a merge/rebase etc, and the file name had got mangled 😄 Makes sense now that you explain it. Good to know, thanks!

/// Gets or sets the arguments.
/// </summary>
/// <value>The arguments.</value>
public Action<ProcessArgumentBuilder> ArgumentCustomization { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if it would make sense to have it be a Func instead if Action instead

public Func<ProcessArgumentBuilder, ProcessArgumentBuilder> ArgumentCustomization { get; set; }

so we could modify and return same instance, with the extension methods:

Settings.ArgumentCustomization = arg=>arg.Append("-true");

,but also just as easily return a new instance/exsisting instance.

Settings.ArgumentCustomization = arg=>"my new arguments";
...
Settings.ArgumentCustomization = arg=>reUseExistingArguments;
...
Settings.ArgumentCustomization = arg=>GetCustomNewArgs(param1, param2);

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add an extension method for this, but just providing an Action is probably simpler and fits the most common use case where you want to add an argument that is missing.

If you completely want to rewrite the arguments, you could simply use the builder.Clear() method before adding your arguments.

Copy link
Member

Choose a reason for hiding this comment

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

I kinda disagree, but might be personal preference.
Personally don't think this is unclean, unclear or less fluent.

Settings.ArgumentCustomization = builder=>"arg"

vs.

Settings.ArgumentCustomization = builder => {  
    builder.Clear();
    builder.Append("Arg");
};

As the all the extension methods return a ProcessArgumentBuilder it would just work without any code change.

I would just find a Func more flexible with little added complexity, would support any scenario the Action does plus a few more.

@patriksvensson patriksvensson changed the title [WIP] Added support for adding custom arguments to tools. Added support for adding custom arguments to tools. Dec 17, 2015
@patriksvensson
Copy link
Member Author

I think this one is ready to be merged 👍

devlead added a commit that referenced this pull request Dec 17, 2015
Added support for adding custom arguments to tools.
@devlead devlead merged commit ce99a1d into cake-build:develop Dec 17, 2015
@devlead devlead mentioned this pull request Dec 17, 2015
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.

3 participants