GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Cleanup Json cmdlets by iSazonov · Pull Request #5001 · PowerShell/PowerShell

Conversation

Ilya (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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) added 4 commits

October 4, 2017 16:12


          Cleanup Json cmdlets from FullCLR code

0e31a27


          Remove ImportJsonDotNetModule

6e31130


          Remove ImportJsonDotNetModule

bc56a61


          Update Newton.Json version on 10.0.3

6dfe2ae

Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw)

Oct 4, 2017

Ilya (iSazonov) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Dan Travison (dantraMSFT) and Dongbo Wang (daxian-dbw) as code owners

October 4, 2017 13:31


          Remove ImportJsonDotNetModule 2

075fe73

Steve Lee (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

Steve Lee (SteveL-MSFT) Oct 4, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) approved these changes

Oct 4, 2017

View reviewed changes

Dan Travison (dantraMSFT) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) approved these changes

Oct 4, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (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 sergei (@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.

sergei (@vors) Dongbo Wang (@daxian-dbw) Could you please comment too? Is it safe?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (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.

Ilya (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [Feature] Fix newline at EOF

3d99be8

Ilya (iSazonov) force-pushed the cleanup-json branch from a4ef026 to 3d99be8 Compare

October 6, 2017 03:21

Dongbo Wang (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"

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) merged commit 50f6667 into PowerShell:master

Oct 6, 2017

Ilya (iSazonov) deleted the cleanup-json branch

October 6, 2017 19:00

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Cleanup Json cmdlets (PowerShell#5001)

dbdf0a2

Reviewers

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

Travis Plunk (TravisEz13)

Awaiting requested review from TravisEz13

+1 more reviewer

Dan Travison (dantraMSFT)

dantraMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.