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

Enable module/resource syntax and improve file error reporting #3186

Merged
merged 6 commits into from
Apr 27, 2023

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Apr 25, 2023

Change

This change adds a new syntax that can be used when referencing resources in a configuration file: Module/Resource. This is only added to a new 0.2 schema version, mainly to prevent an issue with older clients failing to run the newer files in a way that is less obvious than "unrecognized configuration file version".

As part of adding a new schema version, I also added the schema version as a property to the ConfigurationSet and ConfigurationUnit. This will enable the processor to handle any semantic differences that might arise as schemas evolve.

Finally, the result from opening a configuration file now contains the field name and value, as well as the line and column number for the error (all values supplied as appropriate). All of this information is better presented to the user from the family of winget configure commands.

Validation

New tests are added to validate the result of opening a configuration file.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner April 25, 2023 22:12
// The line number for the failure reason, if determined.
UInt32 Line{ get; };

// The line number for the failure reason, if determined.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: line column

{
AICLI_LOG(Config, Error, << "ConfigurationSetParser error: " << AppInstaller::Logging::SetHRFormat << result << " [" << field << "]");
return hstring{ L"0.1" };
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be 0.2?

return std::make_unique<ConfigurationSetParserError>(WINGET_CONFIG_ERROR_UNKNOWN_CONFIGURATION_FILE_VERSION, GetFieldName(FieldName::ConfigurationVersion), versionNode.as<std::string>());
}

bool ConfigurationSetParser::IsRecognizedSchemaVersion(hstring value) try
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe there can be something like an enum that has the recognized versions, unknown and max. a helper method that returns the string or semantic representation and compare with the value given. then Create and current callers of IsRecognizedSchemaVersion throw if the result is unknown and LatestVersion just uses max. but maybe is too much.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think an enum is warranted here, and I would only expect the value to be set in the event that the caller knows what they are doing with it (and therefore knows the versions).

QualifiedResourceName(hstring input)
{
std::wstring_view inputView = input;
size_t pos = inputView.find('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

should we fail if there are more than one / ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that / isn't a valid character in module or resource names, I don't think I mind if we break non-functional files at a later date.

@JohnMcPMS JohnMcPMS merged commit 937ec87 into microsoft:master Apr 27, 2023
@JohnMcPMS JohnMcPMS deleted the config-improv branch April 27, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants