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

Fix MakeRelative regression in v16.10 #6508

Closed
wants to merge 5 commits into from

Conversation

dsparkplug
Copy link

Fixes #6493

Replaces PR by @pmisik : #6504

Context

Recent change to MakeRelative function was causing existing targets to fail. The change removed the backslash from the end of paths. This change causes the function to revert to the previous behaviour.

Changes Made

FileUtilities.cs updated

Testing

Added unit tests to FileUtilities_Tests.cs.

@dnfadmin
Copy link

dnfadmin commented Jun 2, 2021

CLA assistant check
All CLA requirements met.

@dnfadmin
Copy link

dnfadmin commented Jun 2, 2021

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ dsparkplug sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

Assert.Equal(@"def\", FileUtilities.MakeRelative(@"c:\abc\", @"c:\abc\def\"));
Assert.Equal(@"..\", FileUtilities.MakeRelative(@"c:\abc\def\xyz\", @"c:\abc\def\"));
Assert.Equal(@"..\ttt\", FileUtilities.MakeRelative(@"c:\abc\def\xyz\", @"c:\abc\def\ttt\"));
Assert.Equal(@".", FileUtilities.MakeRelative(@"c:\abc\def\", @"c:\abc\def\"));
Copy link

Choose a reason for hiding this comment

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

There is opportunity to extend test coverage for combination of file+directory and directory+file.

/* Directory + File */
 Assert.Equal(@"def", FileUtilities.MakeRelative(@"c:\abc\", @"c:\abc\def"));
 Assert.Equal(@"..\..\def", FileUtilities.MakeRelative(@c:\abc\def\xyz\", @"c:\abc\def"));
 Assert.Equal(@"..\ghi", FileUtilities.MakeRelative(@c:\abc\def\xyz\", @"c:\abc\def\ghi"));
 Assert.Equal(@"..\def", FileUtilities.MakeRelative(@c:\abc\def\", @"c:\abc\def"));

/* File + Directory */
 Assert.Equal(@"def\", FileUtilities.MakeRelative(@c:\abc", @"c:\abc\def\"));
 Assert.Equal(@"..\", FileUtilities.MakeRelative(@c:\abc\def\xyz", @"c:\abc\def\"));
 Assert.Equal(@"..\ghi\", FileUtilities.MakeRelative(@c:\abc\def\xyz", @"c:\abc\def\ghi\"));
 Assert.Equal(@"", FileUtilities.MakeRelative(@c:\abc\def", @"c:\abc\def\"));

Copy link
Author

Choose a reason for hiding this comment

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

Good suggestion - have added more tests

@dsparkplug dsparkplug changed the base branch from vs16.11 to vs16.10 June 2, 2021 13:23
@dsparkplug
Copy link
Author

dsparkplug commented Jun 2, 2021

Hmmm.. didn't expect all those commits to be added to the pull request when changing the base branch. And the checks failed too - changing back...

@dsparkplug dsparkplug changed the base branch from vs16.10 to vs16.11 June 2, 2021 13:31
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Looks good! I think I know when this came in—the original uri-based implementation failed tests in the installer repo, for some reason. This version passed those tests and was a bit faster—but also inadvertently changed our behavior with slashes. I don't see why that would be necessary to maintain, so I think this is a good change.

src/Shared/FileUtilities.cs Outdated Show resolved Hide resolved
@Forgind
Copy link
Member

Forgind commented Jun 2, 2021

Oh, and also:
#6504 (comment)

🙂

Is there any reason to keep #6504 around anymore?

Co-authored-by: Forgind <Forgind@users.noreply.github.com>
@dsparkplug
Copy link
Author

dsparkplug commented Jun 2, 2021

The other pr was started by @pmisik - it looks like he has pulled my changes into that and rebased to v16.10 - but it doesn't build for some reason. I really should have based this pr on v16.10 as the fix is needed to fix major dependency issues. Will attempt to base it on v16.10 now - do I need to rebase the source before using the new update to base branch feature?

@dsparkplug
Copy link
Author

Have created a new PR #6513 with the same changes squashed and rebased onto vs16.10 branch

@dsparkplug dsparkplug closed this Jun 3, 2021
@dsparkplug dsparkplug deleted the vs16.11 branch June 3, 2021 02:22
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.

4 participants