GH899: Copy directory structure #1145
Conversation
|
Hi @maddin2016, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
Hi there, is this request from interest or can close this? |
I don't think any of the team have had a chance to look at this yet, but that doesn't mean that we are not interested, just that we are busy elsewhere. In order to help with reviewing this, would it be possible for you to rebase on the latest develop branch, and also fix up the build errors that are associated with this PR? |
|
@maddin2016 sorry for not getting back to you sooner, currently we're fully focused on releasing Cake.CoreCLR for the v0.16.0 release so we haven't had an chance to review this yet. I can see the all CI servers failed could you look at attending that? |
|
@maddin2016 looks like your not mocking filesystem and doing operations against the actual file system, so that could be an lead what's failing, see here: |
|
|
Is this |
|
@maddin2016 You need to use FakeFileSystem when writing unit tests against the file system since they shouldn't touch the actual file system. |
|
Mmh, that test |
|
i have commented above that this test is only a placeholder because currently it is not possible to simulate this test. |
| var newAbsoluteTargetPath = newTargetPath + @"/" + filePath.GetFilename(); | ||
| context.Log.Verbose("Copying file {0} to {1}", absoluteFilePath.GetFilename(), newAbsoluteTargetPath); | ||
|
|
||
| if (!Directory.Exists(newTargetPath)) |
There was a problem hiding this comment.
You should not use System.IO here. Use the IFileSystem available on the provided context.
|
@maddin2016 I reviewed the code in the current state and left some comments for you. |
|
@maddin2016 your using System.IO directly in a few places instead of Cake's abstractions, this is undesired not only because of tests but also makes it impossible to replace functionality via Cake's module support like i.e. the Cake LongPath Module does. A couple of places:
Use IFileySystem GetDirectory available as FileSystem on ICakeContext IDirectory exposes an Exists via the IFileSystemInfo interface. IDirectory aslo exposes an Create method. DirectoryPath provides an GetDirectoryName() |
|
Thanks for that tips!!! |
|
To do
|
martinscholz83
force-pushed
the
copy-directory-structure
branch
from
8e0f02b
to
5975039
Compare
|
@patriksvensson, can you please have a look at my last commits. Thanks! |
|
@maddin2016 I'm busy until Monday evening but perhaps someone else in the @cake-build/team can take a look at this tomorrow or during the weekend. |
|
Hi @patriksvensson, do you had the chance to look at the commits? |
martinscholz83
force-pushed
the
copy-directory-structure
branch
from
53e7d7b
to
a4119fb
Compare
|
@maddin2016 would you be in a position to squash commits and rebase against latest develop? |
|
No problem. Should i squash into one commit? |
|
@maddin2016 if you could, yes, that would be great. |
martinscholz83
force-pushed
the
copy-directory-structure
branch
2 times, most recently
from
4bcafc4
to
6df96dc
Compare
|
Looks like there is some encoding problem with tests!? And i struggle currently with another problem. If i try to use i get the following error |
|
@maddin2016 As we already have |
That's because i have set them with a default value so you can still use the overloads as before. But apparently that is not working 😕 So i will add the old overloads an let them call the new one's with default |
|
The new Overloads are
and
So why i'm not able to call |
|
@maddin2016 Removing the old methods and adding new ones with an optional parameter will break application binary compatibility with aany addin/module/application/etc. depending on those methods. To not do that add new methods with the If you do this it will just work and you won't break existing addins/modules/applications as old method IL signature will still exist. |
|
It maybe that this error comes from Roslyn compiler? Error happens when evaluate the script https://github.com/cake-build/cake/blob/develop/src/Cake/Scripting/XPlat/DefaultXPlatScriptSession.cs#L40 I have tested it with a dummy Alias If you not explicitly set the parameter |
|
@devlead, i didn't see your comment. Ok, i will revert my changes and add the new overloads in addition. |
|
@maddin2016 excellent 👍 |
martinscholz83
force-pushed
the
copy-directory-structure
branch
2 times, most recently
from
ee2699b
to
81e79b3
Compare
|
@maddin2016 could you please rebase against latest develop so we can do a final review before merging. |
martinscholz83
force-pushed
the
copy-directory-structure
branch
from
81e79b3
to
8232e43
Compare
* fixes # 899
devlead
force-pushed
the
copy-directory-structure
branch
from
8232e43
to
12fd0ed
Compare
|
@maddin2016 your changes have now been merged, thanks for contributing! 👍 |
|
Your'e welcome 👍 |
|
How does this exactly work?
I'd like to overwrite the 2 files in the |
fix #899
Here is an approach to keep the folder structure for
CopyFiles. I add some new overloads. Yet only for patterns.