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

RevEng: Make some APIs public (non-Internal) #10517

Merged
merged 1 commit into from
Dec 12, 2017
Merged

RevEng: Make some APIs public (non-Internal) #10517

merged 1 commit into from
Dec 12, 2017

Conversation

bricelam
Copy link
Contributor

@bricelam bricelam commented Dec 8, 2017

This exposes the entry point into the Reverse Engineer functionality (IReverseEngineerScaffolder); the contract for code generators (IModelCodeGenerator); and the service for selecting a code generator (IModelCodeGeneratorSelector).

It also adds extension methods for adding our design-time services to an IServiceCollection. (fixes #9339) You can use them like this:

var designTimeServiceCollection = new ServiceCollection()
    .AddEntityFrameworkDesignTimeServices()
    .AddDbContextDesignTimeServices(dbContext);
new SqlServerDesignTimeServices().ConfigureDesignTimeServices(designServiceCollection);

var designTimeServiceProvider = designTimeServiceCollection.BuildServiceProvider();

var migrationsScaffolder = designTimeServiceProvider.GetService<IMigrationsScaffolder>();
// migrationsScaffolder.ScaffoldMigration(...)
// migrationsScaffolder.RemoveMigration(...)

var reverseEngineerScaffolder = designTimeServiceProvider.GetService<IReverseEngineerScaffolder>();
// reverseEngineerScaffolder.ScaffoldModel(...)

This cleans up some other DI contracts too.

cc @dazinator @ErikEJ @tonysneed

@tonysneed
Copy link

@bricelam Have you thought about moving EntityEntryGraphIterator out of Internal? My framework depends on it

@bricelam
Copy link
Contributor Author

bricelam commented Dec 9, 2017

@ajcvickers? (make EntityEntryGraphIterator public)

@ajcvickers
Copy link
Member

@tonysneed Do you depend on EntityEntryGraphIterator or IEntityEntryGraphIterator?

@bricelam
Copy link
Contributor Author

bricelam commented Dec 11, 2017

@tonysneed FYI, I imagined EntityFrameworkCore.Scaffolding.Handlebars would override IModelCodeGeneratorSelector since it's template-based and thus language-agnostic. Meanwhile, the VB and F# generators will override IModelCodeGenerator.

@tonysneed
Copy link

@ajcvickers I'm creating a new EntityEntryGraphIterator. The code looks like this:

public static void TraverseGraph(this DbContext context, object item,
    Action<EntityEntryGraphNode> callback)
{
    IStateManager stateManager = context.Entry(item).GetInfrastructure().StateManager;
    var node = new EntityEntryGraphNode(stateManager.GetOrCreateEntry(item), null, null);
    IEntityEntryGraphIterator graphIterator = new EntityEntryGraphIterator();
    var visited = new HashSet<int>();

    graphIterator.TraverseGraph(node, n =>
    {
        // Check visited
        if (visited.Contains(n.Entry.Entity.GetHashCode()))
            return false;

        // Execute callback
        callback(n);

        // Add visited
        visited.Add(n.Entry.Entity.GetHashCode());

        // Return true if node state is null
        return true;
    });
}

The reason for my own TraverseGraph method is that setting EntityState on TrackGraph halts recursion, and there are scenarios where I need to set the state but continue recursion.

// if outputDir is a subfolder of projectDir, then use each subfolder as a subnamespace
// --output-dir $(projectFolder)/A/B/C
// => "namespace $(rootnamespace).A.B.C"
private string SubnamespaceFromOutputPath(string projectDir, string outputDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these args converted to full path specification rather than relative paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do a bit verification in this area before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

projectDir is. outputDir is as it was entered by the user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will normalize outputDir here.

/// <summary>
/// Represents the files added for a model.
/// </summary>
public class ModelFiles
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we review these names before release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We'll cover in API review

@smitpatel
Copy link
Contributor

    public class FakeAnnotationCodeGenerator : IAnnotationCodeGenerator

Could this override from base class?


Refers to: test/EFCore.Design.Tests/Scaffolding/Internal/ReverseEngineeringConfigurationTests.cs:52 in bdcc70b. [](commit_id = bdcc70b, deletion_comment = False)

@smitpatel
Copy link
Contributor

public class ReverseEngineeringConfigurationTests

Should change name of this test file


Refers to: test/EFCore.Design.Tests/Scaffolding/Internal/ReverseEngineeringConfigurationTests.cs:16 in bdcc70b. [](commit_id = bdcc70b, deletion_comment = False)

Copy link
Contributor

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

:shipit:

@bricelam
Copy link
Contributor Author

Agree the tests could use a bit more cleanup. I'll address as part of #9111

@ajcvickers
Copy link
Member

@tonysneed For the not stopping on state set thing, we have an issue to fix this: #8226. It's good additional feedback that you would use this.

About the code you posted, is there a reason not to resolve IEntityEntryGraphIterator from the context D.I. rather than newing it up explicitly? For example, var graphIterator = context.GetService<IEntityEntryGraphIterator>();

(I'm asking these questions so as to determine if it is best just to make the service public or also the default implementation of the service. Generally we only make the default implementation public if it is expected that it act as a base class for customized implementations.)

@tonysneed
Copy link

For the not stopping on state set thing, we have an issue to fix this: #8226. It's good additional feedback that you would use this.

I'll comment over there for clarification.

About the code you posted, is there a reason not to resolve IEntityEntryGraphIterator from the context D.I. rather than newing it up explicitly? For example, var graphIterator = context.GetService<IEntityEntryGraphIterator>();

It didn't dawn on me I could use the context as a service locator. I'll refactor the code to do that. It would be nice to have IEntityEntryGraphIterator lifted out of the Internal namespace, but I don't think I have the need to extend EntityEntryGraphIterator.

@ajcvickers
Copy link
Member

ajcvickers commented Dec 12, 2017

@tonysneed Thanks. Filed #10540 to track making IEntityEntryGraphIterator public.

This exposes the entry point into the Reverse Engineer functionality (IReverseEngineerScaffolder); the contract for code generators (IModelCodeGenerator); and the service for selecting a code generator (IModelCodeGeneratorSelector).

It also adds extension methods for adding our design-time services to an IServiceCollection. (fixes #9339)

This cleans up some other DI contracts too.
@bricelam bricelam merged commit bf1afac into dotnet:dev Dec 12, 2017
@bricelam bricelam deleted the uint branch December 12, 2017 18:33
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.

4 participants