Added null-value checks for collections and attributes.#10
Open
pcleckler wants to merge 2 commits intoCloudNimble:masterfrom
Open
Added null-value checks for collections and attributes.#10pcleckler wants to merge 2 commits intoCloudNimble:masterfrom
pcleckler wants to merge 2 commits intoCloudNimble:masterfrom
Conversation
robertmclaws
requested changes
Jun 21, 2018
Contributor
robertmclaws
left a comment
robertmclaws
left a comment
There was a problem hiding this comment.
Hey, thanks so much for this contribution. I have some comments inline, and if we can get these adjustments made, we can roll it out to users very quickly.
Again, thanks so much!
| <Generator>VsixManifestGenerator</Generator> | ||
| <LastGenOutput>source.extension.resx</LastGenOutput> | ||
| </None> | ||
| <None Include="VersionKeeper.config" /> |
Contributor
There was a problem hiding this comment.
Please remove this from the project.
| <EmbedInteropTypes>False</EmbedInteropTypes> | ||
| </Reference> | ||
| <Reference Include="System" /> | ||
| <Reference Include="System.Configuration" /> |
Contributor
There was a problem hiding this comment.
Please remove this from the project.
| </Target> | ||
| <Import Project="..\packages\Microsoft.VSSDK.BuildTools.15.1.192\build\Microsoft.VSSDK.BuildTools.targets" Condition="'$(VisualStudioVersion)' != '14.0' And Exists('..\packages\Microsoft.VSSDK.BuildTools.15.1.192\build\Microsoft.VSSDK.BuildTools.targets')" /> | ||
| <Import Project="..\packages\Microsoft.VisualStudio.Sdk.BuildTasks.14.0.14.0.215\build\Microsoft.VisualStudio.Sdk.BuildTasks.14.0.targets" Condition="'$(VisualStudioVersion)' == '14.0' And Exists('..\packages\Microsoft.VisualStudio.Sdk.BuildTasks.14.0.14.0.215\build\Microsoft.VisualStudio.Sdk.BuildTasks.14.0.targets')" /> | ||
| <PropertyGroup> |
Contributor
There was a problem hiding this comment.
Please remove this section from the project.
| @@ -0,0 +1,14 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
Contributor
There was a problem hiding this comment.
Please remove this file from the project.
| @@ -0,0 +1,25 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
Contributor
There was a problem hiding this comment.
Please remove this file from the project.
| <Identity Id="bae2a4ae-be17-4f34-be32-f7f103918589" Version="2018.6.20.3" Language="en-US" Publisher="Robert McLaws" /> | ||
| <DisplayName>NuGet PackageReference Upgrader</DisplayName> | ||
| <Description xml:space="preserve">A VS2017 Extension that helps legacy apps migrate off of packages.config. </Description> | ||
| <MoreInfo>https://github.com/pcleckler/PackageReferenceUpgrader</MoreInfo> |
Contributor
There was a problem hiding this comment.
Please remove this from the project. I will identify your contribution elsewhere.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> | ||
| <Metadata> | ||
| <Identity Id="bae2a4ae-be17-4f34-be32-f7f103918589" Version="1.0.0" Language="en-US" Publisher="Robert McLaws" /> |
Contributor
There was a problem hiding this comment.
Please undo this change.
|
|
||
| //RWM: Remove the old Standard Reference. | ||
| oldReferences.Where(c => c.Attribute("Include").Value.Split(new Char[] { ',' })[0].ToLower() == row.Attribute("id").Value.ToLower()).ToList() | ||
| if (oldReferences != null) oldReferences.Where(c => c.Attribute("Include") != null).Where(c => c.Attribute("Include").Value.Split(new Char[] { ',' })[0].ToLower() == row.Attribute("id").Value.ToLower()).ToList() |
Contributor
There was a problem hiding this comment.
Were there exceptions happening here? Because of the way LINQ works, I assumed that oldReferences would be a blank collection 100% of the time.
Two observations:
- There is only one situation where you should EVER do an if statement without braces: when you're checking parameters to short-circuit a function. (
if (someParameter == null) return;), If you go outside of that rule, you risk creating the exact same issue that affected OpenSSL. - This is great, and I really appreciate it. Wouldn't this be easier by just using a null conditional operator (
oldReferences?.Where(...))?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Could not get this to work in my VB.NET project until I added these checks. And apologies if this pull request generated work (forgive a newbie). Changes to NuGetUpgraderPackage.cs are the only necessary changes to the master branch of the head fork.