Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vcpkg] Further JSON error improvements #13399

Merged
merged 19 commits into from
Oct 12, 2020

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Sep 7, 2020

This PR does a few things:

  1. Internally splits the representation (json.h) and parser (jsonreader.h) to reduce unintended dependencies across the codebase. The same refactor is also applied to Configuration/ConfigurationDeserializer.
  2. Introduces levenshtein-distance suggestions for misspelled fields.
  3. Minor tweak to the identifier error message.

Potential future work includes applying this levenshtein-distance calculation to search results.

As a drive by, this PR documents that comment fields ($xyz) are not supported in objects being used as sets with user-defined keys; they are only allowed in "struct-like" objects with a defined keyspace.

Example 1

{ "name": "Abseil", ... }
PS C:\src\vcpkg\testing> ..\vcpkg-old.exe install --feature-flags=manifests,registries --dry-run
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.name: mismatched type: expected an identifier
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.
PS C:\src\vcpkg\testing> ..\vcpkg.exe install --feature-flags=manifests,registries --dry-run
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.name (an identifier): must be lowercase alphanumeric+hyphens and not reserved
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

Example 2

{ .., "depednecies": ["foo"], ... }
PS C:\src\vcpkg\testing> ..\vcpkg-old.exe install --feature-flags=manifests,registries --dry-run
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $ (a manifest): unexpected field 'depednecies'
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.
PS C:\src\vcpkg\testing> ..\vcpkg.exe install --feature-flags=manifests,registries --dry-run
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $ (a manifest): unexpected field 'depednecies', did you mean 'dependencies'?
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

Example 3

{
  ...,
  "features": {
    "$abc" : true
  }
}
PS C:\src\vcpkg\testing> ..\vcpkg-old.exe install --feature-flags=manifests,registries --dry-run
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.features (a features field): unexpected field '$abc'
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.
PS C:\src\vcpkg\testing> ..\vcpkg.exe install --feature-flags=manifests,registries --dry-run
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.features (a set of features): unexpected field '$abc': must be lowercase alphanumeric+hyphens and not reserved
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

@PhoebeHui PhoebeHui added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Sep 8, 2020
@strega-nil strega-nil self-requested a review September 8, 2020 17:02
@strega-nil
Copy link
Contributor

Putting myself as a required reviewer.

toolsrc/include/vcpkg/base/fwd/view.h Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/fwd/json.h Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/fwd/json.h Outdated Show resolved Hide resolved
toolsrc/include/vcpkg/base/jsonreader.h Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/json.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/configuration.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/registries.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/registries.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/sourceparagraph.cpp Show resolved Hide resolved
toolsrc/src/vcpkg/sourceparagraph.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

Just a few things left :)

toolsrc/src/vcpkg/base/json.cpp Show resolved Hide resolved
toolsrc/src/vcpkg/sourceparagraph.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/sourceparagraph.cpp Show resolved Hide resolved
@PhoebeHui PhoebeHui added the info:internal This PR or Issue was filed by the vcpkg team. label Sep 17, 2020
@ras0219-msft ras0219-msft merged commit d1ba685 into microsoft:master Oct 12, 2020
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
* [vcpkg] Split vcpkg/base/json.h into vcpkg/base/jsonreader.h

* [vcpkg] Extract definitions of Configuration-Deserializer (& friends)

These types are only used by VcpkgPaths during the initial parse.

* [vcpkg] Introduce levenshtein-distance suggestions for json errors

* [vcpkg] Fix regression in supports handling

* [vcpkg] Fix signed/unsigned mismatch

* [vcpkg] Address CR comments

* [vcpkg] Address CR comments

* Fix compiler error from merge conflict.

* [vcpkg] Change parameters of Reader::check_for_unexpected_fields to better match declaration

* [vcpkg] Improve errors from features set

* [vcpkg] Fix includes

* [vcpkg] Reuse code

* [vcpkg] Check the "name" field always to maximize error information

* [docs] Improve english phrasing in manifests.md

* [vcpkg] Correct docs link for manifests

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants