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

Closes #486: CopyFile logs incorrect targetFilePath #487

Merged
merged 1 commit into from
Nov 6, 2015

Conversation

jrnail23
Copy link
Contributor

@jrnail23 jrnail23 commented Nov 4, 2015

No description provided.

@devlead
Copy link
Member

devlead commented Nov 5, 2015

Looks sold @jrnail23 👍 , could you get the branch up to date and I'll merge.

@devlead devlead added this to the v0.6.1 milestone Nov 5, 2015
@jrnail23 jrnail23 force-pushed the fix-copyfile-logging branch from b7b5f62 to 3ed4531 Compare November 5, 2015 22:32
@jrnail23
Copy link
Contributor Author

jrnail23 commented Nov 5, 2015

@devlead, should be ready to go now, pending CI build

@patriksvensson
Copy link
Member

Restarted Linux build that had timed out when restoring NuGet packages.

@jrnail23
Copy link
Contributor Author

jrnail23 commented Nov 5, 2015

@patriksvensson, did it happen again?

@patriksvensson
Copy link
Member

@jrnail23 Hmm. Yes 😢 I'll restart it again.

I should really try to get the packages properly cached on Travis since this fail in more than 50% of the builds.

var fixture = new FileCopyFixture();

// When
FileAliases.CopyFile(fixture.Context, "./file1.txt", "./target/file1.txt");
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for nitpicking, but a better test would be to test for ./file1.txt and ./target/file2.txt.
If I understand correctly the bug will output the filename of the target file first, which would give the same results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but the fixture isn't very straightforward in terms of changing that part. I did, however, start from a failing test.
I can still try to get that to work if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. I think it's OK then 😄

@patriksvensson
Copy link
Member

This looks good to me 👍
Ok to merge?

@patriksvensson patriksvensson self-assigned this Nov 5, 2015
@patriksvensson
Copy link
Member

@jrnail23 Looks like this needs to be rebased against develop.

@jrnail23 jrnail23 force-pushed the fix-copyfile-logging branch from 3ed4531 to 94ca297 Compare November 6, 2015 01:45
@jrnail23
Copy link
Contributor Author

jrnail23 commented Nov 6, 2015

@patriksvensson, rebased again, ready to go (pending CI build)

devlead added a commit that referenced this pull request Nov 6, 2015
Closes #486: CopyFile logs incorrect targetFilePath
@devlead devlead merged commit af62492 into cake-build:develop Nov 6, 2015
@devlead
Copy link
Member

devlead commented Nov 6, 2015

Merged @jrnail23, thanks for contributing 👍

@jrnail23 jrnail23 deleted the fix-copyfile-logging branch November 6, 2015 01:51
@jrnail23
Copy link
Contributor Author

jrnail23 commented Nov 6, 2015

👍

@gep13 gep13 removed this from the v0.6.1 milestone Nov 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants