Fixed loading of PackagePart relationships to avoid (incorrect) conversion of relative target uris into absolute. (fixes xamarin bug #6602) [Take II] #482

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

@pruiz
Contributor
pruiz commented Oct 20, 2012

Also, this pull-request enables unit-tests contained at PackUriHelperTests.cs which had a class-level IgnoreAttribute due to some not implemented features at PackUriParser which are now fixed.

See: https://bugzilla.xamarin.com/show_bug.cgi?id=6602

@pruiz pruiz Fixed loading of PackagePart relationships to avoid (incorrect) conve…
…rsion of relative target uris into absolute. (fixes xamarin bug #6602)
90bb2f1
@alanmcgovern alanmcgovern commented on an outdated diff Dec 10, 2012
...lass/WindowsBase/System.IO.Packaging/PackUriParser.cs
@@ -64,35 +64,37 @@ protected override string GetComponents(Uri uri, UriComponents components, UriFo
builder.Length == 0)
start++;
- builder.Append(s, start, end - start);
+ if (start > 0) builder.Append(s, start, end - start);
}
@alanmcgovern
alanmcgovern Dec 10, 2012 Contributor

There are a bunch of tab characters here instead of spaces which ruins the formatting of the file. Would you be able to fix this?

@alanmcgovern alanmcgovern commented on an outdated diff Dec 10, 2012
...lass/WindowsBase/System.IO.Packaging/PackUriParser.cs
- int fragIndex = s.IndexOf('#');
- int end = fragIndex == -1 ? s.Length : fragIndex;
- builder.Append(s, index, end - index);
+ int fragIndex = s.IndexOf('#');
+ int end = fragIndex == -1 ? s.Length : fragIndex;
+ builder.Append(s, index, end - index);
+ }
}
@alanmcgovern
alanmcgovern Dec 10, 2012 Contributor

There are a bunch of tab characters here instead of spaces which ruins the formatting of the file. Would you be able to fix this?

@alanmcgovern alanmcgovern commented on an outdated diff Dec 10, 2012
...wsBase/Test/System.IO.Packaging/PackUriHelperTests.cs
@@ -97,6 +98,13 @@ public void CreateTest ()
new Uri ("/main.html", UriKind.Relative), "#frag").ToString (), "#3");
}
+ [Test]
+ public void CreateTest2()
+ {
+ Uri uri = PackUriHelper.Create(new Uri("http://www.test.com/pack1.pkg"));
+ Assert.AreEqual("pack://pack:,,http:%2C%2Cwww.test.com%2Cpack1.pkg,/", PackUriHelper.Create(uri).ToString());
+ }
+
[Test]
@alanmcgovern
alanmcgovern Dec 10, 2012 Contributor

The indentation is also weird for the [Category] attributes and some of the new tests

@alanmcgovern alanmcgovern was assigned Dec 10, 2012
@alanmcgovern
Contributor

Hey, sorry for the delay in responding. I saw your original pull request but for some reason I either did not get any notifications when this one was created or simply missed them.

There are a bunch of places in the commit where there are tabs instead of spaces which makes the file look very strange. Would you be able to harmonise the new code so it uses the same indentation method as the rest of the file? I'll merge the commit once that's done.

@pruiz
Contributor
pruiz commented Dec 10, 2012

Sure, I'll look at it over the weekend, and will add a new patch fixing withespace changes.

@alanmcgovern
Contributor

Great, thanks! When you fix it up can you ping me with it. If you write '@alanmcgovern' in a message i should definitely get a notification. I'll keep an eye on the repo just in case though

@pruiz
Contributor
pruiz commented Dec 16, 2012

Hi @alanmcgovern

I just pushed a couple of patches fixing whitespace changes.. it was a bit weird as it looks like some files use tabs while others use spaces.. nevermind ;)

Greets
Pablo

@alanmcgovern
Contributor

I fetched those commits and ran the testsuite. We have 28 failures with the patch applied. I don't think any of them are regressions, I think it's just the tests that were re-enabled that are failing. I'll investigate the issue and merge the changes if there has been no noticeable regression.

@alanmcgovern
Contributor
@alanmcgovern
Contributor

Just so you know, I squashed your three commits into a single commit and pushed that to mono/master. The squashed commit is here: a0395f9

I did an additional commit after that to disable a few more failing tests in PackUriHelper so the test suite is green again. Thanks for the patch, it's definitely fixed some bugs and it's great that we are running at least some of the tests in PackUriHelper now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment