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

Refactor IPackage #4174

Merged
merged 12 commits into from Feb 27, 2024
Merged

Refactor IPackage #4174

merged 12 commits into from Feb 27, 2024

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Feb 16, 2024

Change

To better reflect the underlying code and continue to move business logic out of the CompositeSource, this change refactors the IPackage interface into 3:

    // Contains a collection of package versions.
    struct IPackageVersionCollection
    {
        // Gets all versions of this package.
        // The versions will be returned in sorted, descending order.
        //  Ex. { 4, 3, 2, 1 }
        virtual std::vector<PackageVersionKey> GetVersionKeys() const = 0;

        // Gets a specific version of this package.
        virtual std::shared_ptr<IPackageVersion> GetVersion(const PackageVersionKey& versionKey) const = 0;

        // A convenience method to effectively call `GetVersion(GetVersionKeys[0])`.
        virtual std::shared_ptr<IPackageVersion> GetLatestVersion() const = 0;
    };

    // Contains information about a package and its versions from a single source.
    struct IPackage : public IPackageVersionCollection
    {
        // Gets a property of this package.
        virtual Utility::LocIndString GetProperty(PackageProperty property) const = 0;

        // Gets the source that this package is from.
        virtual Source GetSource() const = 0;

        // Determines if the given IPackage refers to the same package as this one.
        virtual bool IsSame(const IPackage*) const = 0;

        // Gets this object as the requested type, or null if it is not the requested type.
        virtual const void* CastTo(IPackageType type) const = 0;
    };

    // Contains information about the graph of packages related to a search.
    struct ICompositePackage
    {
        // Gets a property of this package result.
        virtual Utility::LocIndString GetProperty(PackageProperty property) const = 0;

        // Gets the installed package information.
        virtual std::shared_ptr<IPackage> GetInstalled() = 0;

        // Gets all of the available packages for this result.
        // There will be at most one package per source in this list.
        virtual std::vector<std::shared_ptr<IPackage>> GetAvailable() = 0;
    };

IPackageVersionCollection is simply a set of IPackageVersion objects with only the previous IPackage functions to enumerate them. This enables some code that only wants to inspect versions to use a tighter set of APIs and thus some implementations need not worry about things like the appropriate property values to return.

The refactored IPackage inherits from IPackageVersionCollection and is itself a set of package versions that all come from a single source (as in Source). It adds the properties and the Source that the versions are all from.

ICompositePackage is the new search result, resembling the previous IPackage in what it contains but exposing it more directly. The installed IPackage object will eventually be able to contain more than one installed version in a future change. The available packages are exposed directly as IPackage objects, rather than mixing the versions together in the CompositeSource.

The code in PackageVersionSelection.cpp extracts the business logic that was inside CompositeSource previously, as well as providing a few helpers to make the existing code work more succinctly. The refactoring is not attempting to change behavior, but simply make it possible to change behavior more easily and precisely in the future.

I would have preferred to go further by splitting CompositeSource out into a public type, allowing the other sources to have a simpler model of only ever returning IPackage results from search. This change would have created an even larger churn though and did not directly contribute towards the goal of supporting future side-by-side work.
  
Also changes the YAML object model to use a non-exceptional function to attempt to convert to an integer.

Validation

Existing regression tests should cover all changes.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner February 16, 2024 04:33
florelis
florelis previously approved these changes Feb 16, 2024
@@ -358,7 +358,8 @@ namespace AppInstaller::CLI::Configuration
{
if (!package.Version.empty())
{
auto versionKeys = searchResult.Matches.at(0).Package->GetAvailableVersionKeys();
std::shared_ptr<Repository::IPackage> availablePackage = searchResult.Matches.at(0).Package->GetAvailable().at(0);
Copy link
Member

Choose a reason for hiding this comment

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

Package->GetAvailable().at(0)
What if the version we're looking for is not in the first available source?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code requires a source name, and at this time there is no way to name a composite source. Thus, there will only ever be one available package.

As I mentioned in the description, this is a case where separating CompositeSource out from ISource would be very helpful. But that change would be even larger.

OutputCompletionString(stream, vc.Version);
if (word.empty() || Utility::ICUCaseInsensitiveStartsWith(vc.Version, word))
{
OutputCompletionString(stream, vc.Version);
Copy link
Member

Choose a reason for hiding this comment

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

Does the order in which these are output matter?

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 think we should set a goal of output in "best match order"; aka the order that places the most likely completion first and so forth. I believe that PowerShell's implementation does use the order as the tab completion cycle.

I assume what you are indicating is that the order will change when there are multiple available packages. This is true, but I'm not sure that is incorrect in this case.

@@ -140,7 +140,7 @@ namespace AppInstaller::CLI::Workflow
}
else
{
auto availableVersion = package->GetAvailableVersion({ pinKey.SourceId, "", "" });
auto availableVersion = GetAvailableVersionsForInstalledVersion(package, nullptr)->GetVersion({ pinKey.SourceId, "", "" });
Copy link
Member

Choose a reason for hiding this comment

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

Could this be GetAvailablePackageFromSource(package, pinKey.sourceId)->GetLatestVersion()?

@@ -628,7 +629,7 @@ namespace AppInstaller::CLI::Workflow

for (size_t i = 0; i < searchResult.Matches.size(); ++i)
{
auto latestVersion = searchResult.Matches[i].Package->GetLatestAvailableVersion();
auto latestVersion = GetAvailableVersionsForInstalledVersion(searchResult.Matches[i].Package, nullptr)->GetLatestVersion();
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a GetAvailableVersions(ICompositePackage) so that we don't have to pass nullptr?

@@ -327,163 +340,127 @@ namespace AppInstaller::Repository
std::shared_ptr<IPackageVersion> m_trackingPackageVersion;
};

// A composite package for the CompositeSource.
struct CompositePackage : public IPackage
// A composite package for the installed package of a CompositePackage.
Copy link
Member

Choose a reason for hiding this comment

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

"A composite package for [...] a CompositePackage"? Should it just be "A package for [...]"?

// A composite package for the CompositeSource.
struct CompositePackage : public ICompositePackage
{
CompositePackage(const std::shared_ptr<ICompositePackage>& installedPackage, const std::shared_ptr<ICompositePackage>& availablePackage = {})
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add comment that availablePackage can only include one package?

This comment has been minimized.

florelis
florelis previously approved these changes Feb 26, 2024
Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@JohnMcPMS JohnMcPMS merged commit c5fbdd9 into microsoft:master Feb 27, 2024
8 checks passed
@JohnMcPMS JohnMcPMS deleted the package-shake branch February 27, 2024 17:47
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

3 participants