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

remove unused code #2609

Merged
merged 4 commits into from Aug 17, 2017

Conversation

Projects
None yet
2 participants
@forki
Member

forki commented Aug 12, 2017

No description provided.

@forki forki requested a review from matthid Aug 12, 2017

forki added some commits Aug 12, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member

Yes the unused code was a oversight temporarily used for debugging, nice catch.

Did you profile/measure the switch to hashset? I'm asking because the list is always quite small, so the hashset might not even be optimal. At least this code did not yield any alarm bells in the profiler at the time...

Member

matthid commented Aug 12, 2017

Yes the unused code was a oversight temporarily used for debugging, nice catch.

Did you profile/measure the switch to hashset? I'm asking because the list is always quite small, so the hashset might not even be optimal. At least this code did not yield any alarm bells in the profiler at the time...

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member
1) Failed : Paket.LockFile.ParserSpecs.should parse lock file many frameworks
  Expected: "NUGET
  remote: https://www.nuget.org/api/v2
    CommonServiceLocator (1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+monoandroid+xamarinios+win8+wp8+wpa81)
    MvvmLightLibs (5.2)
      CommonServiceLocator (>= 1.0) - restriction: || (== net35) (== sl4)
      CommonServiceLocator (>= 1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+monoandroid+xamarinios+win8+wp8+wpa81)"
Actual: "NUGET
  remote: https://www.nuget.org/api/v2
    CommonServiceLocator (1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+win8+wp8+wpa81)
    MvvmLightLibs (5.2)
      CommonServiceLocator (>= 1.0) - restriction: || (== net35) (== sl4)
      CommonServiceLocator (>= 1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+win8+wp8+wpa81)"
  Expected string length 406 but was 360. Strings differ at index 135.
  Expected: "...(>= net40) (>= portable-net45+monoandroid+xamarinios+win8+..."
  But was:  "...(>= net40) (>= portable-net45+win8+wp8+wpa81)\r\n    MvvmLig..."

This is actually correct as you can see in http://nugettoolsdev.azurewebsites.net/4.0.0/parse-framework?framework=portable-net45%2Bmonoandroid%2Bxamarinios%2Bwin8%2Bwp8%2Bwpa81

Not exactly sure how that PR fixes the portable detection.

Same for

3) Failed : Paket.LockFile.ParserSpecs.should parse portable lockfile
  Expected: ">= portable-net40+win8+wp8+sl5"
Actual: ">= portable-net40+sl5+win8+wp8"
  String lengths are both 30. Strings differ at index 18.
  Expected: ">= portable-net40+win8+wp8+sl5"
  But was:  ">= portable-net40+sl5+win8+wp8"
  -----------------------------^
at FsUnit.Extensions.shouldEqual[a](a expected, a actual) in C:\projects\paket\paket-files\test\forki\FsUnit\FsUnit.fs:line 10

I'm not exactly sure about


2) Failed : Paket.LockFile.ParserSpecs.should parse new restrictions || (&& (< net45) (>= portable-net45+win8+wp8+wpa81)) (&& (< portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81) (>= portable-net45+win8+wp8+wpa81))
...
  Expected string length 1344 but was 1023. Strings differ at index 308.
  Expected: "...t.Bcl (1.1.10) - restriction: || (&& (< net45) (>= portabl..."
  But was:  "...t.Bcl (1.1.10) - restriction: && (< net45) (>= portable-ne..."
  --------------------------------------------^
at FsUnit.Extensions.shouldEqual[a](a expected, a actual) in C:\projects\paket\paket-files\test\forki\FsUnit\FsUnit.fs:line 10

But maybe paket now detects that one of the platforms is a proper subset of the other... On the other hand it might be a incorrect detection of portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81 to one of the default profiles. As NuGet cannot assign a default portable profile (http://nugettoolsdev.azurewebsites.net/4.0.0/parse-framework?framework=portable-net45%2Bmonoandroid%2Bmonotouch%2Bxamarinios%2Bxamarinmac%2Bwin8%2Bwp8%2Bwpa81) We should not do it as well.

Member

matthid commented Aug 12, 2017

1) Failed : Paket.LockFile.ParserSpecs.should parse lock file many frameworks
  Expected: "NUGET
  remote: https://www.nuget.org/api/v2
    CommonServiceLocator (1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+monoandroid+xamarinios+win8+wp8+wpa81)
    MvvmLightLibs (5.2)
      CommonServiceLocator (>= 1.0) - restriction: || (== net35) (== sl4)
      CommonServiceLocator (>= 1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+monoandroid+xamarinios+win8+wp8+wpa81)"
Actual: "NUGET
  remote: https://www.nuget.org/api/v2
    CommonServiceLocator (1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+win8+wp8+wpa81)
    MvvmLightLibs (5.2)
      CommonServiceLocator (>= 1.0) - restriction: || (== net35) (== sl4)
      CommonServiceLocator (>= 1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+win8+wp8+wpa81)"
  Expected string length 406 but was 360. Strings differ at index 135.
  Expected: "...(>= net40) (>= portable-net45+monoandroid+xamarinios+win8+..."
  But was:  "...(>= net40) (>= portable-net45+win8+wp8+wpa81)\r\n    MvvmLig..."

This is actually correct as you can see in http://nugettoolsdev.azurewebsites.net/4.0.0/parse-framework?framework=portable-net45%2Bmonoandroid%2Bxamarinios%2Bwin8%2Bwp8%2Bwpa81

Not exactly sure how that PR fixes the portable detection.

Same for

3) Failed : Paket.LockFile.ParserSpecs.should parse portable lockfile
  Expected: ">= portable-net40+win8+wp8+sl5"
Actual: ">= portable-net40+sl5+win8+wp8"
  String lengths are both 30. Strings differ at index 18.
  Expected: ">= portable-net40+win8+wp8+sl5"
  But was:  ">= portable-net40+sl5+win8+wp8"
  -----------------------------^
at FsUnit.Extensions.shouldEqual[a](a expected, a actual) in C:\projects\paket\paket-files\test\forki\FsUnit\FsUnit.fs:line 10

I'm not exactly sure about


2) Failed : Paket.LockFile.ParserSpecs.should parse new restrictions || (&& (< net45) (>= portable-net45+win8+wp8+wpa81)) (&& (< portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81) (>= portable-net45+win8+wp8+wpa81))
...
  Expected string length 1344 but was 1023. Strings differ at index 308.
  Expected: "...t.Bcl (1.1.10) - restriction: || (&& (< net45) (>= portabl..."
  But was:  "...t.Bcl (1.1.10) - restriction: && (< net45) (>= portable-ne..."
  --------------------------------------------^
at FsUnit.Extensions.shouldEqual[a](a expected, a actual) in C:\projects\paket\paket-files\test\forki\FsUnit\FsUnit.fs:line 10

But maybe paket now detects that one of the platforms is a proper subset of the other... On the other hand it might be a incorrect detection of portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81 to one of the default profiles. As NuGet cannot assign a default portable profile (http://nugettoolsdev.azurewebsites.net/4.0.0/parse-framework?framework=portable-net45%2Bmonoandroid%2Bmonotouch%2Bxamarinios%2Bxamarinmac%2Bwin8%2Bwp8%2Bwpa81) We should not do it as well.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member

Let me know if that makes any sense at all :)

Member

matthid commented Aug 12, 2017

Let me know if that makes any sense at all :)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 12, 2017

Member
Member

forki commented Aug 12, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member

Ah now I see.

This is really weird, maybe a nuget bug.
"portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81" -> No detection

But if the non pcl stuff is removed:

"portable-net45+win8+wp8+wpa81" -> Profile 259

I think we would need to ask a NuGet expert about this.
If we want to mirror NuGet it will be tough without understanding the underlying logic...

Member

matthid commented Aug 12, 2017

Ah now I see.

This is really weird, maybe a nuget bug.
"portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81" -> No detection

But if the non pcl stuff is removed:

"portable-net45+win8+wp8+wpa81" -> Profile 259

I think we would need to ask a NuGet expert about this.
If we want to mirror NuGet it will be tough without understanding the underlying logic...

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

Once it's green obviously ;)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 12, 2017

Member
Member

forki commented Aug 12, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member

What do you mean? Updating the tests?

Member

matthid commented Aug 12, 2017

What do you mean? Updating the tests?

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member

Maybe remove the last commit

Member

matthid commented Aug 12, 2017

Maybe remove the last commit

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 12, 2017

Member
Member

forki commented Aug 12, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 12, 2017

Member

Yes strictly speaking they are probably a bugfix for some non-existing pcl users ;)

Member

matthid commented Aug 12, 2017

Yes strictly speaking they are probably a bugfix for some non-existing pcl users ;)

Revert "Use sets"
This reverts commit 49164d4.
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 12, 2017

Member

Ok. Reverted sets

Member

forki commented Aug 12, 2017

Ok. Reverted sets

@forki forki merged commit 7069575 into master Aug 17, 2017

0 of 4 checks passed

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