All CLA requirements met.

I'm not sure if this is a problem, but I notice that all of the tests are written with single quotes. Newtonsoft is happy to parse these, but System.Text.Json will not. I suspect this is the source the test failures, but I'm not sure. I could use some help getting the tests to run locally.

@gregsdennis Yes, it is main breaking change in System.Text.Json.

@SteveL-MSFT Do you agree to begin migration to System.Text.Json in the 7.4 milestone?

There seem to be a lot more changes to files.wxs that I expected. Maybe this is normal?

"invalid spacing" - but what should it be? It tells me its wrong but not how to fix it.

@gregsdennis Please finish replacing ' with " in tests since you have started the process.

Invalid spacing

Try reformat:

var validationResults = _jschema.Validate(parsedJson, new ValidationOptions
{
    OutputFormat = OutputFormat.Basic
});

> files.wxs

You need to build win package locally and the script will publish new files.wxs in temp folder.

@gregsdennis Maybe #11397 help you a bit.

Please finish replacing ' with " in tests since you have started the process.

I expect you don't mean all the tests. Even just in that file seems overkill. It seems there's a preference to have " wherever possible, so I only changed what was necessary.

Try reformat

The format of Func(new Class { Prop = value }); seems to be working. I can still change to what you have if that's preferred.

Please finish replacing ' with " in tests since you have started the process.

I expect you don't mean all the tests. Even just in that file seems overkill. It seems there's a preference to have " wherever possible, so I only changed what was necessary.

I still see test fails because of "

The failing tests aren't (all) because of ".

For example, Json is valid against a valid schema from file is failing because the lib doesn't know about valid_schema_definitions.json, which is being referenced.

JSON Schema explicitly states that implementations SHOULD NOT download references that appear to be URLs. The URIs that are used are merely identifiers, not locations. As such, downloading is disabled in my lib by default, requiring pre-loading of all required schemas, but it seems that it may have been enabled in NJsonSchema. I can make this change.

I'll go through the other tests to see what the problems are.

As such, downloading is disabled in my lib by default, requiring pre-loading of all required schemas

So we cannot reference base schema file from new schema file?

If PowerShell is comfortable with automatically fetching external documents, it can be enabled, yes, but it's not generally recommended by JSON Schema. Given that this is previous functionality, I think enabling it is fine.

I suggest adding this to the docs as a deviation from the spec.

If PowerShell is comfortable with automatically fetching external documents, it can be enabled, yes, but it's not generally recommended by JSON Schema. Given that this is previous functionality, I think enabling it is fine.

I suggest adding this to the docs as a deviation from the spec.

Since this has not been reported as a security issue until now we should keep current behavior if possible. Then we will ask PowerShell security group.

It looks like there's some syntax issue I don't understand in the Test-Json recognizes non-object types: <name> tests on line 156.

I've extracted the loop into a separate file:

$types = 'string', 'number', 'boolean', 'null', 'array', 'object'
$types | Where-Object {
    $schema = "{ ""type"": ""$_"" }"
    Write-Output $schema
}


$type = 'string'
$schema = "{ ""type"": ""$type"" }"
Write-Output $schema

This outputs

string
number
boolean
null
array
object
{ "type": "string" }

I don't understand why the interpolation isn't working in the loop but it works outside of it.

I don't understand why the interpolation isn't working

PowerShell interpolation works only in double quotes.

I added commit to fix the test.

What's the policy on changing error codes?

Test-Json throw if a schema from file is invalid is expecting InvalidJsonSchema, but the file isn't valid JSON at all. It seems that NJsonSchema preprocesses $refs so that it tries to deserialize the text as a schema before it ever reaches the validation stage. JsonSchema.Net deserializes referenced data (and caches it) at validation time. This means that the command reaches the validation stage and the resulting error is InvalidJson instead (which I think is actually more correct).

In both cases, it's an error, but the exact error is different.

Edit: After the commit below, all the tests are passing except for this.

This means that the command reaches the validation stage and the resulting error is InvalidJson instead (which I think is actually more correct).

I agree with InvalidJson since we check just Json. However, the user must understand the root of the error whether it is the Json itself or a problem with the schema (reading/parsing). In other words InvalidJson must contain InvalidJsonSchema as an inner exception and the test should check this.

I've rebased and pushed.

@iSazonov since the update to .Net 7 RC1, I can't build anymore. I've installed the preview framework (Windows x64 installer).

I'm getting a lot of errors saying that it can't find Microsoft.CodeAnalysis.

An instance of analyzer Microsoft.CodeAnalysis.MakeFieldReadonly.MakeFieldReadonlyDiagnosticAnalyzer
cannot be created from C:\Program Files\dotnet\sdk\7.0.100-rc.1.22431.12\Sdks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CodeStyle.dll :
Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'
or one of its dependencies. The system cannot find the file specified.

It seems there are some rebase conflicts with how I merged the .wxs file.

I'm getting a lot of errors saying that it can't find Microsoft.CodeAnalysis.

Usually we need to remove nuget cache in current user profile to fix such issues. Also you need to run once Start-PSBuild -Clean.

@iSazonov I've regen'd the files.wxs file (and removed the pdb's) but I'm not sure it's right. There seem to be other DLLs listed that I didn't do anything with.

@iSazonov I've regen'd the files.wxs file (and removed the pdb's) but I'm not sure it's right. There seem to be other DLLs listed that I didn't do anything with.

Please build msi package locally and use files.wxs from the build.

Please build msi package locally and use files.wxs from the build.

Are there instructions for this? All I can find is the Windows build instructions. This only generates an .exe.

https://github.com/PowerShell/PowerShell/blob/7dc4587014bfa22919c933607bf564f0ba53db2e/.github/CONTRIBUTING.md#pull-request---automatic-checks

Import-Module .\build.psm1
Start-PSBuild -Clean -CrossGen -PSModuleRestore -Runtime win7-x64 -Configuration Release -ReleaseTag <release tag>
Import-Module .\tools\packaging
Start-PSPackage -Type msi -ReleaseTag <release tag> -WindowsRuntime 'win7-x64' -SkipReleaseChecks

Console output should contain a reference to new files.wxs.

GitHub

PowerShell/CONTRIBUTING.md at 7dc4587014bfa22919c933607bf564f0ba53db2e · PowerShell/PowerShell

PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

@iSazonov I ran that except that it didn't recognize the -CrossGen switch, so I had to remove that.

PS C:\projects\PowerShell> Start-PSBuild -Clean -CrossGen -PSModuleRestore -Runtime win7-x64 -Configuration Release -ReleaseTag v1.0.0-local
Start-PSBuild: A parameter cannot be found that matches parameter name 'CrossGen'.

It also added all of the .pdb files, so I removed those. It still doesn't match. 😭

This PR has 209 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Label      : Large
Size       : +125 -84
Percentile : 60.9%

Total files changed: 5

Change summary by file extension:
.wxs : +30 -22
.csproj : +1 -1
.cs : +59 -26
.ps1 : +35 -35

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@gregsdennis I added new commit to fix packaging.

Thank you @iSazonov

I'm going through the checklist now, and the only breaking change that I can think of is the lack of support for single-quoted JSON (which isn't really JSON anyway).

Does this need to be documented somewhere? If so, where? Do I need to create a separate issue to track that?

Does this need to be documented somewhere? If so, where? Do I need to create a separate issue to track that?

Common rule for PowerShell is to follow .Net behavior. I think it makes sense to explain the breaking change in docs. But now is not the time. The change has to be approved by MSFT team first. Then there are other Json cmdlets that should be migrated. And finally, there are many small differences between NewtonSoft and .Net implementations. I guess we are not able to describe them all. We can only point out this fact. I don't think this is a critical change, because the .Net implementations follow Json standard very closely.

@gregsdennis The PR contains a lot of temporary comments, so I suggest to close the PR and open new one.