Skip to content

Merge ModuleCatalog into ComponentCatalog. #1022

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

Merged
merged 2 commits into from
Sep 25, 2018

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Sep 25, 2018

This completes the ComponentCatalog refactoring work for v0.6. It merges the ModuleCatalog and ComponentCatalog types into a single type.

Follow up for #208.

@@ -290,8 +290,6 @@ public sealed class CmdParser
private const int SpaceBeforeParam = 2;
private readonly ErrorReporter _reporter;
private readonly IHost _host;
// REVIEW: _catalog should be part of environment and can be get through _host.
Copy link
Contributor

Choose a reason for hiding this comment

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

REVIEW: _catalog should be part of environment and can be get through _host. [](start = 11, length = 76)

How about that. :)

public static void Initialize()
{
ImageAnalytics.Initialize();
}
Copy link
Contributor

@TomFinley TomFinley Sep 25, 2018

Choose a reason for hiding this comment

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

What is the new alternative to this, for the sake of the legacy API? @yaeldekel

Copy link
Member Author

Choose a reason for hiding this comment

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

We only have 2 of these empty Initialize() methods, so it wasn't really a holistic plan.
If someone is using the "new API" this isn't necessary at all.

If someone is using the legacy API, and needs to do this, they can follow the same approach outlined in the following issues:

Basically, they can just get the typeof some type in that assembly to ensure it is loaded up front.

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Excellent, thank you @eerhardt . Only real problem is the initialize method I think, that's making your build break but otherwise looks great.

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

@shauheen shauheen merged commit 59a90e7 into dotnet:master Sep 25, 2018
@eerhardt eerhardt deleted the MergeModuleCatalog branch September 25, 2018 23:34
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants