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

GetMatchingFiles() Return Type #63

Closed
WalkerCodeRanger opened this issue Feb 20, 2018 · 3 comments
Closed

GetMatchingFiles() Return Type #63

WalkerCodeRanger opened this issue Feb 20, 2018 · 3 comments
Milestone

Comments

@WalkerCodeRanger
Copy link

The GlobbingExtensions.GetMatchingFiles(ICakeContext, IEnumerable<FilePath>) method returns IEnumerable<IFile> this is inconsistent with similar methods that return FilePathCollection. Note that those are paths rather than files.

Methods returning FilePathCollection:

  • GlobbingExtensions.GetFiles(ICakeContext, string[])
  • GlobbingAliases.GetFiles(ICakeContext, string)
  • GlobbingAliases.GetFiles(ICakeContext, string, Func<IDirectory, bool>)
@wwwlicious
Copy link
Contributor

@WalkerCodeRanger Thanks for contributing 👍

I'm assuming you propose changing this to match the other extensions.
This would be a breaking change for existing clients which I try never to do (especially as many people don't pin versions which is the recommended guidance).

Is there a reason other than consistency across different extensions for making this change that I should consider as otherwise I'm relucant to do so?

@WalkerCodeRanger
Copy link
Author

Yes, I would propose this be changed to match the other ones.

I understand the concern about it being a breaking change. However, if you let inconsistencies like this build up in the system over time, eventually there will be quite a few and it could be frustrating for users. I assumed part of the reason for the Cake.Incubator add-in was to allow for more changes, including breaking changes, before stabilizing the feature into cake. Would it be possible to change it at the time it moves from incubator into cake proper?

As an aside, I think you should change cake to make pinning the version required or at least the default. Personally, I pinned the version the moment I set it up and would never dream of using it not pinned.

@wwwlicious
Copy link
Contributor

wwwlicious commented Feb 21, 2018

@WalkerCodeRanger you can propose pinning versions by default to the cake team in the main repo.

It is the intent to allow for more changes yes, specifically around adoption of things not already in the main cake project where they can mature and stabilize but making a breaking change to a method signature is still not something I like to do without some thought.

I will consider making this breaking change for version 2 in line with semver.

@wwwlicious wwwlicious added this to the 3,0 milestone Aug 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants