Skip to content

Cleanup Json cmdlets #5001

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

Merged
merged 6 commits into from
Oct 6, 2017
Merged

Cleanup Json cmdlets #5001

merged 6 commits into from
Oct 6, 2017

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Oct 4, 2017

Related #4357.

Cleanup Json cmdlets from FullCLR code.
Remove ImportJsonDotNetModule.
Update Newton.Json version 10.0.2 -> 10.0.3


return result;
}
#endif
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a newline since you're already updating this file?

Copy link
Contributor

@dantraMSFT dantraMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.
Is it safe to assume that ImportJsonDotNetModule hasn't been needed for a while?

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 5, 2017

Is it safe to assume that ImportJsonDotNetModule hasn't been needed for a while?

I asked in Gitter and only @vors commented:

Ilya @iSazonov Sep 07 07:16 
I wonder why we explicitly load dll Newtonsoft.Json, Version=7.0.0.0 and why version is 7.0.0?

Sergei Vorobev @vors Sep 07 07:17 
@iSazonov it’s historical reason of the convertfrom-json port for nano server as far as I remember
version is whatever was latest available at the moment and went thru all the testing scrunities
hardcoded version also can be an attemp to allow modules to load a newer versions and not affect the core cmdlets

Also I guess it could have been because of the problems with loading dlls in the PowerShell Core early port phases.

I tried to list loaded assemblies and don't see Newton.Json 7.0 - only 10 version was loaded.

@vors @daxian-dbw Could you please comment too? Is it safe?

@daxian-dbw
Copy link
Member

Yes, it's safe to remove it.
Back in NanoServer time, the only way to ship Newton.json.dll is to wrap it into a module (Json.Net) and load that module when the Json cmdlet is in use. Now we don't even have the Json.Net module.

@iSazonov Can you please change the last commit message to add [Feature] to trigger the full test run? Some Json cmdlet tests are marked with -Tags Feature.

@daxian-dbw
Copy link
Member

There is one known failure. It failed due to a race condition between the test and the test service, not related to this PR. I will merge this PR.

[-] Validate Invoke-RestMethod error with environment proxy set - 'http_proxy' 5.29s
   Expected: {System.Threading.Tasks.TaskCanceledException,Microsoft.PowerShell.Commands.InvokeRestMethodCommand}
   But was:  {}
   at line: 1407 in /Users/travis/build/PowerShell/PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
   1407:         $result.Error.FullyQualifiedErrorId | Should Be "System.Threading.Tasks.TaskCanceledException,Microsoft.PowerShell.Commands.InvokeRestMethodCommand"

@daxian-dbw daxian-dbw merged commit 50f6667 into PowerShell:master Oct 6, 2017
@iSazonov iSazonov deleted the cleanup-json branch October 6, 2017 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants