Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Cleanup Json cmdlets#5001

Merged
daxian-dbw merged 6 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:cleanup-jsoniSazonov/PowerShell:cleanup-jsonCopy head branch name to clipboard
Oct 6, 2017
Merged

Cleanup Json cmdlets#5001
daxian-dbw merged 6 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:cleanup-jsoniSazonov/PowerShell:cleanup-jsonCopy head branch name to clipboard

Conversation

@iSazonov

@iSazonov iSazonov commented Oct 4, 2017

Copy link
Copy Markdown
Collaborator

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
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Outdated
}
#endif
}
} No newline at end of file

@SteveL-MSFT SteveL-MSFT Oct 4, 2017

Copy link
Copy Markdown
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?

dantraMSFT
dantraMSFT approved these changes Oct 4, 2017
View reviewed changes

@dantraMSFT dantraMSFT left a comment

Copy link
Copy Markdown
Contributor

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

@SteveL-MSFT SteveL-MSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@iSazonov

iSazonov commented Oct 5, 2017

Copy link
Copy Markdown
Collaborator Author

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

daxian-dbw commented Oct 5, 2017

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

daxian-dbw commented Oct 6, 2017

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

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.