Skip to content

Improve invalid feature name reporting #1675

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.

Sign up for GitHub

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

Merged
BillyONeal merged 1 commit into from
May 8, 2025
Merged

Improve invalid feature name reporting #1675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions azure-pipelines/e2e-assets/format-manifest-malformed/bad-feature-name.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"name": "bad-feature-name",
"version": "0",
"features": {
"a": {
"description": "Feature A"
},
"b_required": {
"description": "This feature has an invalid name"
},
"c": {
"description": "Feature C"
}
}
}
5 changes: 5 additions & 0 deletions azure-pipelines/end-to-end-tests-dir/format-manifest.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ $testProjects | % {
}
}

$output = Run-VcpkgAndCaptureOutput @commonArgs format-manifest "$PSScriptRoot/../e2e-assets/format-manifest-malformed/bad-feature-name.json"
Throw-IfNotFailed
$expected = "bad-feature-name.json: error: $.features.b_required (a feature): features must be lowercase alphanumeric+hyphens, and not one of the reserved names"
Throw-IfNonContains -Expected $expected -Actual $output

Write-Trace "test re-serializing every manifest"
$manifestDir = "$TestingRoot/manifest-dir"
Copy-Item -Path "$env:VCPKG_ROOT/ports" -Destination $manifestDir -recurse -Force -Filter vcpkg.json
Expand Down
1 change: 1 addition & 0 deletions include/vcpkg/base/jsonreader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ namespace vcpkg::Json
void add_expected_type_error(const LocalizedString& expected_type);
void add_extra_field_error(const LocalizedString& type, StringView fields, StringView suggestion = {});
void add_generic_error(const LocalizedString& type, StringView message);
Comment on lines 47 to 48
Copy link

Copilot AI May 8, 2025

Choose a reason for hiding this comment

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

[nitpick] It would be beneficial to add a comment describing the purpose of add_field_name_error and how it differs from add_generic_error.

Suggested change
void add_extra_field_error(const LocalizedString& type, StringView fields, StringView suggestion = {});
void add_generic_error(const LocalizedString& type, StringView message);
void add_extra_field_error(const LocalizedString& type, StringView fields, StringView suggestion = {});
// Reports a general error related to the JSON object, not tied to a specific field.
void add_generic_error(const LocalizedString& type, StringView message);
// Reports an error related to a specific field name in the JSON object.
// Use this method when the error pertains to a particular field, such as a missing or invalid value.

Copilot uses AI. Check for mistakes.
void add_field_name_error(const LocalizedString& type, StringView field, StringView message);

void add_warning(LocalizedString type, StringView msg);

Expand Down
6 changes: 6 additions & 0 deletions src/vcpkg/base/json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1492,6 +1492,12 @@ namespace vcpkg::Json
.append_raw(message));
}

Copy link

Copilot AI May 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding inline documentation here to explain that add_field_name_error wraps the generic error logic while providing additional context from the field name.

Suggested change
// This function wraps add_generic_error while temporarily appending the field name to the path.
// It provides additional context by including the field name in the error message.

Copilot uses AI. Check for mistakes.
void Reader::add_field_name_error(const LocalizedString& type, StringView field, StringView message)
{
PathGuard guard{m_path, field};
add_generic_error(type, message);
}

void Reader::check_for_unexpected_fields(const Object& obj,
View<StringLiteral> valid_fields,
const LocalizedString& type_name)
Expand Down
3 changes: 2 additions & 1 deletion src/vcpkg/sourceparagraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,8 @@ namespace vcpkg
}
if (!Json::IdentifierDeserializer::is_ident(pr.first))
{
r.add_generic_error(type_name(), msg::format(msgInvalidFeature));
r.add_field_name_error(
FeatureDeserializer::instance.type_name(), pr.first, msg::format(msgInvalidFeature));
continue;
}
std::unique_ptr<FeatureParagraph> v;
Expand Down
Loading