Skip to content

Conversation

@gregsdennis
Copy link
Contributor

@gregsdennis gregsdennis commented May 4, 2023

PR Summary

Follow-up to #18141. Updates JsonSchema.Net to latest version: v3.3.2 -> v4.1.0.

Release notes can be viewed here.

PR Context

While not strictly required now, this PR sets up for later updates that will incorporate additional support for JSON Schema.

PR Checklist

<value>Cannot parse the JSON.</value>
</data>
<data name="InvalidJsonAgainstSchemaDetailed" xml:space="preserve">
<value>The JSON is not valid with the schema: {0} at {1}</value>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I updated, I tested this locally with a schema & instance that had a problem at the root. The error that's output just ends with "at", and the fact that there's an empty JSON Pointer indicating the root object isn't conveyed. I think the quotes help.

@gregsdennis
Copy link
Contributor Author

gregsdennis commented May 4, 2023

Looking into test failures...

Edit: I know what the problem is. I can update tomorrow.

@gregsdennis
Copy link
Contributor Author

gregsdennis commented May 4, 2023

Actually, I'm not sure what's going on. I think it may have something to do with running in Linux, but I can't be sure. Everything works in Windows and in WSL.

Windows:
image

WSL:
image

This is without further changes.


Note: I'm leaving on business tomorrow for a little over a week, so this PR may need to sit for a while.

@gregsdennis
Copy link
Contributor Author

@daxian-dbw would you mind if I put some temporary Console.WriteLine()s in the code to get some debugging info from the server test runner? I'll remove them once I get it working.

@daxian-dbw
Copy link
Member

@gregsdennis Please feel free to add debugging messages in the code or test.

@iSazonov
Copy link
Collaborator

iSazonov commented May 5, 2023

@gregsdennis Perhaps it is Unix line end chars?

@gregsdennis
Copy link
Contributor Author

No, it's having trouble finding the files (resolving $ref). I'm debugging in #19617.

@gregsdennis gregsdennis changed the title Update jsonschemanet version Update JsonSchema.Net version May 5, 2023
@gregsdennis
Copy link
Contributor Author

Okay. That looks good. Now I just need to figure out this packaging stuff.

@iSazonov I think you just fixed this for me before, but I don't know what you did. I'm happy to learn. Is there any documentation on it? The error just says,

Please update D:\a\1\PowerShell\tools\packaging\Boms\windows.json per the above instructions

but there are no instructions. I assume they're in the code somewhere?

@daxian-dbw
Copy link
Member

Hi @gregsdennis, thanks for finding the root cause and fix the issue!
For the packaging failure, please do the following:

  1. click the "View more details on Azure Pipelines" of the "PowerShell-Windows-Packaging-CI" job
  2. click on the "4 published; 1 consumed" in the "Related" tab
  3. download the 1z5wd3xl.nu4-windows-fyiwyr3s.zry-bom.json file, and then in your working branch, replace the PowerShell\tools\packaging\Boms\windows.json file with it (make sure use the same windows.json name).
  4. commit and push the change.

@gregsdennis
Copy link
Contributor Author

gregsdennis commented May 5, 2023

Thanks, @daxian-dbw, I don't think I could have found that on my own.

Weird. It looks like nuget is packaging my Spanish language pack now. I'll have to look into that.

I had to update VS at some point in this process, and then I published a new version of the package.

@pull-request-quantifier-deprecated

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


Quantification details

Label      : Extra Small
Size       : +18 -16
Percentile : 13.6%

Total files changed: 4

Change summary by file extension:
.csproj : +1 -1
.cs : +14 -14
.resx : +1 -1
.json : +2 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detected.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.


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

@gregsdennis
Copy link
Contributor Author

@daxian-dbw everything looks good from my side. Thanks for the help. Let me know if there's anything else you want me to change.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @gregsdennis for your contribution!

@StevenBucher98 StevenBucher98 added the PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update label May 8, 2023
@daxian-dbw daxian-dbw merged commit 4b261ec into PowerShell:master May 8, 2023
@daxian-dbw daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 8, 2023
@gregsdennis
Copy link
Contributor Author

@daxian-dbw @iSazonov I see releases for 7.2/7.3 just went out and 7.4-preview is also available, but the release notes don't have anything about this.

Just curious when this change is planned to release. Thanks.

@iSazonov
Copy link
Collaborator

@gregsdennis The commit is not backported to 7.2/7.3. (And will not be). The commit will be in 7.4 Preview4.

@gregsdennis
Copy link
Contributor Author

Thank you!

@ghost
Copy link

ghost commented Jun 29, 2023

🎉v7.4.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Extra Small PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants