-
Notifications
You must be signed in to change notification settings - Fork 288
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
Split functions to avoid 'is_manifest' control coupling. #1229
Conversation
@@ -306,8 +307,10 @@ namespace vcpkg | |||
if (!manifest_exists && !control_exists) | |||
{ | |||
msg::write_unlocalized_text_to_stdout(Color::error, fmt::format("FAIL: {}\n", port_name)); | |||
errors.emplace( | |||
msg::format(msgPortMissingManifest, msg::package_name = port_name, msg::path = port_path)); | |||
errors.emplace(LocalizedString::from_raw(port_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this pattern will be common enough to warrant a
LocalizedString LocalizedString::error_in_path(StringView path) {
return LocalizedString::from_raw(path).append_raw(": ").append(msgErrorMessage);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be worth it but I'd rather do it as its own PR rather than expanding the scope of this one
@@ -433,37 +434,47 @@ namespace vcpkg::Paragraphs | |||
auto manifest_contents = fs.read_contents(manifest_path, ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this function should be able to be written with less nesting, perhaps along the lines of:
std::error_code ec_manifest, ec_control;
auto content_manifest = fs.read_contents(path_manifest, ec_manifest);
const auto exists_manifest = !ec_manifest;
const auto failed_manifest = ec_manifest && ec_manifest != std::errc::no_such_file_or_directory;
auto content_control = fs.read_contents(path_control, ec_control);
const auto exists_control = !ec_control;
const auto failed_control = ec_control && ec_control != std::errc::no_such_file_or_directory;
if (failed_manifest) {
// msgFailedToAccessManifest(path_manifest)
} else if (failed_control) {
// msgFailedToAccessManifest(path_control)
} else if (exists_manifest && exists_control) {
// msgManifestConflict(port_directory)
} else if (exists_manifest) {
// success on manifest
} else if (exists_control) {
// success on control
} else {
// msgPortMissingManifest2(port_directory)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this transformation as it does a read_contents
rather than an exists check on the CONTROL
in the case we know loading the json succeeded. I inverted the ec
tests and that flattened the result substantially, does that resolve your hearburn?
Extracted from #1210