Skip to content

Cleanup Json cmdlets#5001

Merged
daxian-dbw merged 6 commits intoPowerShell:masterfrom
iSazonov:cleanup-json
Oct 6, 2017
Merged

Cleanup Json cmdlets#5001
daxian-dbw merged 6 commits intoPowerShell:masterfrom
iSazonov:cleanup-json

Conversation

@iSazonov
Copy link
Copy Markdown
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

SteveL-MSFT
SteveL-MSFT reviewed Oct 4, 2017
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Outdated
}
#endif
}
} No newline at end of file
Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT Oct 4, 2017

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?

dantraMSFT
dantraMSFT approved these changes Oct 4, 2017
View reviewed changes
Copy link
Copy Markdown
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?

SteveL-MSFT
SteveL-MSFT approved these changes Oct 4, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

daxian-dbw commented Oct 5, 2017

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
Copy Markdown
Member

daxian-dbw commented Oct 6, 2017

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

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

+1 more reviewer

@dantraMSFT dantraMSFT dantraMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw daxian-dbw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iSazonov @daxian-dbw @SteveL-MSFT @dantraMSFT