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 package id parsing and avoid NPE when feed is missing some propertie... #776

Merged
merged 4 commits into from Apr 30, 2015

Conversation

Projects
None yet
2 participants
@chillitom
Copy link
Contributor

commented Apr 29, 2015

...s

Nuget feed's <m:properties> doesn't contain a <d:Id>, the id is instead found in the 's <title> element.

The element in a nuget feed contains something like "http://www.nuget.org/api/v2/Packages(Id='_Atrico.Lib.CommonAssemblyInfo',Version='1.0.0.0')" which is not what we want here.

Some nuget package sources omit some elements, for example ProGet doesn't report the Language element for handle missing elements by returning an empty string.

Fix package id parsing and avoid NPE when feed is missing some proper…
…ties

Nuget feed's <m:properties> doesn't contain a <d:Id>, the id is instead found in the <entry>'s <title> element.

The <id> element in a nuget feed contains something like "http://www.nuget.org/api/v2/Packages(Id='_Atrico.Lib.CommonAssemblyInfo',Version='1.0.0.0')" which is not what we want here.

Some nuget package sources omit some elements, for example ProGet doesn't report the Language element for handle missing elements by returning an empty string.
@forki

This comment has been minimized.

Copy link
Member

commented Apr 29, 2015

Hey that sounds good. But we should use id tag whenever it is there

@chillitom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2015

I haven't seen any feeds that contain an d:Id tag, the <id> atom element is not what we want.. the nuget.org stream doesn't appear to have them either.. am I missing something though?

@chillitom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2015

Fyi, just in the process of putting together a testcase

@chillitom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2015

There isn't an F# tests project from what I can see, am I okay to add one?

@forki

This comment has been minimized.

Copy link
Member

commented Apr 29, 2015

Yep.
On Apr 29, 2015 2:52 PM, "chillitom" notifications@github.com wrote:

There isn't an F# tests project from what I can see, am I okay to add one?


Reply to this email directly or view it on GitHub
#776 (comment).

@chillitom chillitom force-pushed the chillitom:patch-1 branch from cf548ed to 713801a Apr 29, 2015

@chillitom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2015

Had difficulty getting a new test project to run as part of the main build so have stuck the new test in the FSCheck.Fake project for now.

I'm remain a little confused as to how the code that was there before worked. Have I done something dumb like reference the wrong stream? It appears to line up with the example in getLatestPackage

@forki

This comment has been minimized.

Copy link

commented on paket.dependencies in 713801a Apr 30, 2015

Why do we need Unquote? why not just use Assert.AreEqual?

This comment has been minimized.

Copy link
Owner Author

replied Apr 30, 2015

We don't need it per se but I found its exception messages much more useful in debugging the issue I found in the function.

For example XUnit's Assert.Equals prints just the ToString of each parameter

Xunit.Sdk.EqualExceptionAssert.Equal() Failure
Expected: FAKE 3.30.0
Actual:   FAKE 3.30.0

Whilst using Unquote prints..

Xunit.Sdk.TrueException{Id = "FAKE";
 Version = "3.30.0";
 Authors = "Steffen Forkmann,  Mauricio Scheffer,  Colin Bull";
 Owners = "Steffen Forkmann,  Mauricio Scheffer,  Colin Bull";
 Url = "http://www.nuget.org/api/v2/package/FAKE/3.30.0";
 IsLatestVersion = true;
 Created = 29/04/2015 07:55:25;
 Published = 29/04/2015 07:55:25;
 PackageHash =
  "tVt0qOybftvi01x3envu9tPO9wErXVHv+zKMay+7h2RRJHIAkA/GgP3XzWlf1tgUQEnd0F/pW+izcSaiOQkG8Q==";
 PackageHashAlgorithm = "SHA512";
 LicenseUrl = "http://www.github.com/fsharp/Fake/blob/master/License.txt";
 ProjectUrl = "http://www.github.com/fsharp/Fake";
 RequireLicenseAcceptance = true;
 Description =
  "FAKE - F# Make - is a build automation tool for .NET. Tasks and dependencies are specified in a DSL which is integrated in F#. This package bundles all extensions.";
 Language = "en-US";
 Tags = "build fake f#";} = {Id = "FAKE";
 Version = "3.30.0";
 Authors = "Steffen Forkmann,  Mauricio Scheffer,  Colin Bull";
 Owners = "Steffen Forkmann,  Mauricio Scheffer,  Colin Bull";
 Url = "http://www.nuget.org/api/v2/package/FAKE/3.30.0";
 IsLatestVersion = true;
 Created = 29/04/2015 07:55:25;
 Published = 29/04/2015 07:55:25;
 PackageHash =
  "tVt0qOybftvi01x3envu9tPO9wErXVHv+zKMay+7h2RRJHIAkA/GgP3XzWlf1tgUQEnd0F/pW+izcSaiOQkG8Q==";
 PackageHashAlgorithm = "SHA512";
 LicenseUrl = "http://www.github.com/fsharp/Fake/blob/master/License.txt";
 ProjectUrl = "http://www.github.com/fsharp/Fake";
 RequireLicenseAcceptance = false;
 Description =
  "FAKE - F# Make - is a build automation tool for .NET. Tasks and dependencies are specified in a DSL which is integrated in F#. This package bundles all extensions.";
 Language = "en-US";
 Tags = "build fake f#";}
false```

Alternately we could test each field individually, would take a bit more code but would give a finer grained exception so maybe worth while.

Let me know what you'd prefer.

This comment has been minimized.

Copy link

replied Apr 30, 2015

I think I would wait a bit with Unqoute. At the moment we have the issue to replace Machine.Specifications and I'm not sure yet which direction we want to go. Please use the xunit default for now.

This comment has been minimized.

Copy link
Owner Author

replied Apr 30, 2015

Okay, I'll rewrite this, obviously it was only a test code dependency but understand not wanting too many differing tools.

Out of interest what are you thinking of replacing MSpec with? Tickspec?

This comment has been minimized.

Copy link

replied Apr 30, 2015

we don't really use it as a BDD tool, so maybe just xunit.

forki added a commit that referenced this pull request Apr 30, 2015

Merge pull request #776 from chillitom/patch-1
Fix package id parsing and avoid NPE when feed is missing some propertie...

@forki forki merged commit 186e987 into fsharp:master Apr 30, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@forki

This comment has been minimized.

Copy link
Member

commented Apr 30, 2015

thanks for this.

@chillitom chillitom deleted the chillitom:patch-1 branch Apr 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.