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 possible null ref when processing a vcxproj file #1814

Merged
merged 1 commit into from Jul 18, 2016

Conversation

Projects
None yet
3 participants
@robertpi
Contributor

robertpi commented Jul 16, 2016

No description provided.

if Path.IsPathRooted path then Path.GetFullPath path else
let di = FileInfo(normalizePath project.FileName).Directory
Path.Combine(di.FullName,path) |> Path.GetFullPath
let optPath =

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

maybe this can be replaced with

let path =
  node
  |> getAttribute "Include" 
  |> Option.map getNormalizedPath

match path with
| None -> ()
| Some path ->
  // make & yield the record

Also since this area of code seems to be tricky, I'd recommend pulling out the expression assigned to that Path record property to an intermediate variable.

@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

maybe this can be replaced with

let path =
  node
  |> getAttribute "Include" 
  |> Option.map getNormalizedPath

match path with
| None -> ()
| Some path ->
  // make & yield the record

Also since this area of code seems to be tricky, I'd recommend pulling out the expression assigned to that Path record property to an intermediate variable.

@smoothdeveloper

This comment has been minimized.

Show comment
Hide comment
@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

@robertpi would you consider adding an integration test with few asserts on expected changes to vcxproj file:

https://github.com/fsprojects/Paket/tree/master/integrationtests/scenarios

adding unit test like you did is good but additional integration test will provide more security regarding overal outcome of running paket restore (or other command, up to you)

Contributor

smoothdeveloper commented Jul 18, 2016

@robertpi would you consider adding an integration test with few asserts on expected changes to vcxproj file:

https://github.com/fsprojects/Paket/tree/master/integrationtests/scenarios

adding unit test like you did is good but additional integration test will provide more security regarding overal outcome of running paket restore (or other command, up to you)

@forki forki merged commit 08d0ef3 into fsprojects:master Jul 18, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment