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

Extend NuGet aliases #662

Conversation

ErikSchierboom
Copy link
Contributor

This PR extends the NuGet aliases to be more flexible. For example, you can now use globbing where appropriate. The only problem with this PR is the fact that there are no tests for the NuGet aliases. That would probably require some slightly complex integration tests, but maybe we should attempt something like that to prevent regression bugs? I've done my best testing the code, but I'd really like other people to also test it.

I was also wondering why there are no overloads for NuGetPack that don't take a NuGetPackSettings. Is there a special reason for that, or should I also add those overloads?

@gep13
Copy link
Member

gep13 commented Jan 26, 2016

@ErikSchierboom thanks again for your continued contributions to Cake, we (the maintainers) really appreciate the work that you are doing.

In this instance though, having discussed it with the team, I don't think we can accept this contribution as is.

Currently, the NuGetPack alias works like this:

NuGetPack("./hello.txt")

And this will be implicitly cast as a FilePath. With the new overload that you have created, which takes a string, means that this valid path will be converted as a glob pattern instead. The only way to work around this would be to do something like this:

NuGetPack(new FilePath("./hello.txt"))

Which is not as convenient.

Rather then potentially break existing consumers of this alias, we are going to recommend that a different approach be used, but one that still provides the ability to use a Globber pattern. The suggested pattern would be to use the same API as used in the NUnit and XUnit aliases, namely in addition to accepting a FilePath, also accept an IEnumerable which would allow you to do this:

NuGetPack(GetFiles("./**/hello.txt"))

As a result, the API layer for NuGetPack is much simpler, and more inline with the other aliases that exist in Cake.Common.

What are your thoughts on this? If you agree, can you update the changes that you have made in this PR? Thanks again!

/cc @patriksvensson @devlead

@ErikSchierboom
Copy link
Contributor Author

@gep13 Ah, my bad! I'm very sorry, I must have missed that in my enthusiasm. I will modify the PR as suggested. The additional overload should be of type IEnumerable<FilePath> right?

@gep13
Copy link
Member

gep13 commented Jan 26, 2016

@ErikSchierboom said...
I'm very sorry,

absolutely no need to apologise! 😄

@ErikSchierboom said...
The additional overload should be of type IEnumerable<FilePath> right?

Yes, that is correct.

@ErikSchierboom ErikSchierboom changed the title Extend NuGet aliases (including globbing) Extend NuGet aliases Jan 27, 2016
@ErikSchierboom
Copy link
Contributor Author

@gep13 I have updated the PR to match the suggestions. This does look a lot better.

@patriksvensson
Copy link
Member

Merged! Rebased this against develop for you, so GitHub does not recognize it as merged though so I have to close it.

Thanks 👍

@ErikSchierboom ErikSchierboom deleted the nuget-aliases-globbing branch September 4, 2017 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants