-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Rest source addition #765
Rest source addition #765
Conversation
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
|
src/WinGetUtil/packages.config
Outdated
@@ -1,5 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<packages> | |||
<package id="cpprestsdk.v142" version="2.10.15" targetFramework="native" /> | |||
<package id="Microsoft.Windows.CppWinRT" version="2.0.200729.8" targetFramework="native" /> | |||
<package id="Microsoft.Windows.ImplementationLibrary" version="1.0.200519.2" targetFramework="native" /> | |||
</packages> |
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.
The same thing applies to a number (but not all) of the files here.
Tangentially related, the spelling bot is similarly upset about the patterns file below (I can't comment on that file because it isn't part of the PR), although for some reason (I need to investigate) it can't seem to properly count to the last line of the file... #Resolved
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
|
src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj
Outdated
Show resolved
Hide resolved
return versionApi; | ||
} | ||
|
||
std::string GetStringFromJsonStringValue(const json::value& value) |
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.
Return a std::optional<std::string>
to preserve ability to model a missing value. #Closed
std::shared_ptr<const RestSource> sharedThis = shared_from_this(); | ||
for (auto& result : results.Matches) | ||
{ | ||
std::unique_ptr<IPackage> package = std::make_unique<AvailablePackage>(sharedThis, result); |
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.
Should std::move
the result into the package since it will no longer be needed. #Closed
{ | ||
std::string version = GetStringFromJsonStringValue(versionItem.at(Version)); | ||
std::string channel = GetStringFromJsonStringValue(versionItem.at(Channel)); | ||
versionList.emplace_back(VersionAndChannel(version, channel)); |
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.
std::move #Closed
versionList.emplace_back(VersionAndChannel(version, channel)); | ||
} | ||
|
||
PackageInfo packageInfo = PackageInfo(packageId, packageName, publisher); |
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.
std::move #Closed
|
||
if (jsonObject.is_null()) | ||
{ | ||
return std::string(); |
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.
In this case you would want to return {};
to return an empty optional, and probably want to preserve that when you change the function to return std::optional<Manifest>
. #Closed
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
|
constexpr std::string_view PackageIdentifier = "PackageIdentifier"sv; | ||
constexpr std::string_view PackageName = "PackageName"sv; | ||
constexpr std::string_view Publisher = "Publisher"sv; | ||
constexpr std::string_view PackageFamilName = "PackageFamilyName"sv; |
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.
constexpr std::string_view PackageFamilName = "PackageFamilyName"sv; | |
constexpr std::string_view PackageFamilyName = "PackageFamilyName"sv; | |
``` #Resolved |
std::optional<std::string> packageId = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageIdentifier))); | ||
std::optional<std::string> packageName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageName))); | ||
std::optional<std::string> publisher = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(Publisher))); | ||
std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilName))); |
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.
std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilName))); | |
std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilyName))); | |
``` #Resolved |
// Interface to this schema version exposed through IRestClient. | ||
struct Interface : public IRestClient | ||
{ | ||
Interface(std::string restApi); |
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.
std::string [](start = 18, length = 11)
const & #Resolved
return utility::conversions::to_string_t(versionEndpoint); | ||
} | ||
|
||
bool IsStringWhitespace(const std::string& value) |
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.
bool IsStringWhitespace(const std::string& value) [](start = 8, length = 49)
I think we have this utility in AppInstallerStrings.hpp. If not, it's a good idea to move the utility there #Resolved
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.
My bad, I forgot to delete this. I am already using Utiltity::IsStringEmptyOrWhitespace
In reply to: 594712782 [](ancestors = 594712782)
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.
Ah, I saw you already used Utility::IsEmptyOrWhitespace(result) below. Then we can delete this
In reply to: 594712782 [](ancestors = 594712782)
} | ||
} | ||
|
||
Interface::Interface(std::string restApi) |
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.
std::string [](start = 25, length = 11)
const & no need for move below
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.
Actually given how GetRestAPIBaseUri
is written, taking in a value and moving it to that function was correct. I think it is likely not to matter who makes the copy, so any of the patterns could work in practice. But the higher you can move the forced copy up the stack, the more efficient you can be usually.
In reply to: 594717708 [](ancestors = 594717708)
SearchResult result; | ||
|
||
// TODO: Handle continuation token. | ||
HttpClientHelper clientHelper(m_searchEndpoint); |
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.
( [](start = 37, length = 1)
{} #Resolved
Interface::Interface(std::string restApi) | ||
{ | ||
m_restApiUri = GetRestAPIBaseUri(std::move(restApi)); | ||
m_searchEndpoint = GetSearchEndpoint(m_restApiUri); |
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.
m_searchEndpoint [](start = 9, length = 16)
Do we want to save a copy for GetManifestByVersionEndpoint too? #WontFix
|
||
IRestClient::SearchResult Interface::Search(const SearchRequest& request) const | ||
{ | ||
UNREFERENCED_PARAMETER(request); |
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.
UNREFERENCED_PARAMETER(request); [](start = 7, length = 33)
not needed, used below #Resolved
|
||
for (auto& manifestItem : dataArray) | ||
{ | ||
std::optional<std::string> packageId = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageIdentifier))); |
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.
packageId [](start = 39, length = 9)
Make sure to revist these fields as some of them are definitely required.
You can add a todo for now since there'll be more error checking, manifest integrity checkto be performed later and they could potentially be in another file. #Resolved
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 saw you handled bad cases by skipping them, I think that's probably enough.
In reply to: 594722790 [](ancestors = 594722790)
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.
Yes, I will be adding a json parser that will handle the checks in my next PR.
In reply to: 594726878 [](ancestors = 594726878,594722790)
|
||
// Parse json and return Manifest | ||
auto& manifestObject = jsonObject.at(GetJsonKeyNameString(Data)); | ||
UNREFERENCED_PARAMETER(manifestObject); |
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.
nit: you can just (void)jsonObject.at(GetJsonKeyNameString(Data)); So you don't need the UNREFERENCED_PARAMETER below #Resolved
{ | ||
if (details.Type.empty()) | ||
{ | ||
// With more than one source implementation, we will probably need to probe first |
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.
// With more than one source implementation, we will probably need to probe first [](start = 20, length = 81)
nit: is this comment copy pasted? Does not seem very relevant to what the code is doing. #Resolved
{ | ||
AICLI_LOG(Repo, Verbose, << "Downloading manifest"); | ||
Manifest::Manifest manifest = GetReferenceSource()->GetRestClient().GetManifestByVersion( | ||
m_packageInfo.packageIdentifier, m_versionInfo.GetVersion().ToString(), m_versionInfo.GetChannel().ToString()).value(); |
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.
.value() [](start = 130, length = 8)
std::optional check it's set before calling Value() #Resolved
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.
you are right, I will just receive it as std::optional. I will implementing and using this in my next PR.
In reply to: 594733374 [](ancestors = 594733374)
} | ||
else if (versionKey.Version.empty() && versionKey.Channel.empty()) | ||
{ | ||
return GetLatestAvailableVersion(); |
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.
return GetLatestAvailableVersion(); [](start = 20, length = 35)
nit packageVersion = GetLatestAvailableVersion(); for code consistency #Resolved
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.
SearchResult result; | ||
|
||
// TODO: Handle continuation token. | ||
HttpClientHelper clientHelper{ m_searchEndpoint }; |
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.
HttpClientHelper [](start = 8, length = 16)
Are these really so cheap that we should make one for every search?
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.
This PR contains the following changes:
Todos:
Tests:
Microsoft Reviewers: Open in CodeFlow