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.

Sign up for GitHub

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

Jump to bottom

3.0 API Review Feedback #15662

Closed
29 tasks done
bricelam opened this issue May 8, 2019 · 4 comments
Closed
29 tasks done

3.0 API Review Feedback #15662

bricelam opened this issue May 8, 2019 · 4 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone
3.0.0

Comments

@bricelam
Copy link
Contributor

bricelam commented May 8, 2019 •
edited by ajcvickers
Loading

Pass 1:

Pass 2:

  • Review PropertyExtensions/MutablePropertyExtensions.FindPrincipal()
    • Better name? Return IEnumerable or include First in the name?
  • @ajcvickers Rename PropertyExtensions.FindMapping()
    • FindTypeMapping? RelationalTypeMapping? Start with Get?
  • @ajcvickers Add breaking change note for ChangeTracker.TrackGraph() This wasn't a breaking change after all
  • Whatever @AndriySvyryd thinks is best: NavigationEntry.GetTargetEntry() returns null. Do we still want to rename CollectionEntry.GetTargetEntry() to FindEntry()?
  • Review IModelSource.GetModel() break. Would a parameter object help? Can we decouple logging?
  • Make AnnotationNames pubternal (again)
  • Review IConvention metadata. (e.g. IConventionEntityType.AddForeignKey() should take IProperty not IConventionProperty)
  • @smitpatel Move IEntityMaterializerSource to a better namespace
  • @AndriySvyryd Propose a better design for IMutableEntityType.AddIndexedProperty()
  • @ajcvickers Should IConventionSetCustomizer be renamed to Plugin?

Pass 3:

  • @ajcvickers Rename IdentifierHelpers
  • @bricelam Test diffing old model snapshots (e.g. Are old discriminator attributes ignored?)
  • @ajcvickers Should IConstructorBindingFactory.TryBindConstructor take IEntityType instead?
  • @ajcvickers Review IParameterBindingFactories, IParameterBindingFactory, IPropertyParameterBindingFactory & ParameterBindingInfo
    • Should IPropertyParameterBindingFactory.TryBindParameter be Find?
    • Is ParameterBindingInfo.GetValueBufferIndex needed?

Pass 4:

  • Document removal of IQueryable.ToLookupAsync() breaking change was added in 3.0
  • Remove IParameterBindingFactory.Bind(IMutableEntityType) Reviewed and this is used and needed
  • @smitpatel Remove async parameter of IDatabase.CompileQuery() (infer when TResult is Task) Covered by Consider revisiting EF.CompileQuery API #14551

Pass 5:

  • @ajcvickers Query
    • Remove indexMap parameter of IEntityMaterializerSource.CreateMaterializeExpression()
    • Make Microsoft.EntityFrameworkCore.Query.NavigationExpansion and .Visitors namespaces pubternal
    • Make these types public
      • CollectionShaperExpression
      • EntityShaperExpression
  • Make a pass over IList and ISet and consider read-only. Also, check for concrete List
  • Add static factory methods to SqlFunctionExpession
  • Seal NodeType for all extension expessions
  • Consider dependency object for QueryCompliationContext
@bricelam bricelam mentioned this issue May 8, 2019
Merged
@ajcvickers ajcvickers self-assigned this May 10, 2019
@ajcvickers ajcvickers added this to the 3.0.0 milestone May 10, 2019
@ajcvickers
Copy link
Member

@ajcvickers to look at 1 and 3. @roji to look at 4.

@ajcvickers
Copy link
Member

ColumnModification seems fine the way it is.

@bricelam
Copy link
Contributor Author

(updated with 2nd pass)

@bricelam bricelam removed this from the 3.0.0 milestone May 22, 2019
@bricelam bricelam mentioned this issue May 22, 2019
Merged
@ajcvickers ajcvickers added this to the 3.0.0 milestone May 31, 2019
AndriySvyryd added a commit that referenced this issue May 31, 2019
@AndriySvyryd
Make conventions public
f926c5f 
Use a parameter object for all convention constructors and IModelSource.GetModel()
Move ValidationConvention to ProviderConventionSetBuilder

Part of #214
Part of #15662
@AndriySvyryd AndriySvyryd mentioned this issue May 31, 2019
Merged
AndriySvyryd added a commit that referenced this issue May 31, 2019
@AndriySvyryd
Make conventions public
9f2b859 
Use a parameter object for all convention constructors and IModelSource.GetModel()
Move ValidationConvention to ProviderConventionSetBuilder

Part of #214
Part of #15662
AndriySvyryd added a commit that referenced this issue Jun 1, 2019
@AndriySvyryd
Make conventions public
ba7c54a 
Use a parameter object for all convention constructors and IModelSource.GetModel()
Move ValidationConvention to ProviderConventionSetBuilder

Part of #214
Part of #15662
AndriySvyryd added a commit that referenced this issue Jun 1, 2019
@AndriySvyryd
Make conventions public
b6be9f7 
Use a parameter object for all convention constructors and IModelSource.GetModel()
Move ValidationConvention to ProviderConventionSetBuilder

Part of #214
Part of #15662
@AndriySvyryd AndriySvyryd mentioned this issue Jun 6, 2019
Merged
AndriySvyryd added a commit that referenced this issue Jun 6, 2019
@AndriySvyryd
Consolidate AddProperty methods
9e0fb47 
React to API review feedback
Add a way to obsolete event definitions

Part of #15662
@AndriySvyryd AndriySvyryd removed their assignment Jun 6, 2019
@ajcvickers ajcvickers mentioned this issue Jun 20, 2019
Merged
ajcvickers added a commit that referenced this issue Jun 21, 2019
@ajcvickers
Rename IConventionSetCustomizer to IConventionSetPlugin
518fb82 
Part of #15662
@ajcvickers ajcvickers mentioned this issue Jun 21, 2019
Merged
smitpatel added a commit that referenced this issue Jun 21, 2019
@smitpatel
Teeny tiny bugs
195faa0 
Part of #16133
Part of #16144
Resolves #16152
Part of #15662
@smitpatel smitpatel mentioned this issue Jun 21, 2019
Merged
smitpatel added a commit that referenced this issue Jun 21, 2019
@smitpatel
Teeny tiny bugs
ac13adf 
Part of #16133
Part of #16144
Resolves #16152
Part of #15662
ajcvickers added a commit that referenced this issue Jun 21, 2019
@ajcvickers
Rename IConventionSetCustomizer to IConventionSetPlugin
74f9c4d 
Part of #15662
smitpatel added a commit that referenced this issue Jun 21, 2019
@smitpatel
Teeny tiny bugs
bbd690b 
Part of #16133
Part of #16144
Resolves #16152
Part of #15662
smitpatel added a commit that referenced this issue Jun 21, 2019
@smitpatel
Teeny tiny bugs
4ddaff5 
Part of #16133
Part of #16144
Resolves #16152
Part of #15662
@bricelam bricelam mentioned this issue Jun 27, 2019
Merged
@ajcvickers ajcvickers added this to the 3.0.0 milestone Jun 28, 2019
ajcvickers added a commit that referenced this issue Jun 30, 2019
@ajcvickers
API review changes
bfb48de 
Issue #15662
@ajcvickers ajcvickers mentioned this issue Jun 30, 2019
Merged
ajcvickers added a commit that referenced this issue Jun 30, 2019
@ajcvickers
API review changes
63a152b 
Issue #15662
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview7 Jul 2, 2019
ajcvickers added a commit that referenced this issue Jul 14, 2019
@ajcvickers
Add more MusicStore and Identity tests
53c3500 
Covering all the queries MusicStore does, and also updating the tests to properly use store-generated values.

Also, added the ModelSnapshots generated by the project templates for 2.1, 2.2, and 3.0, for SQL Server and SQLite. They all diff correctly against the current code showing no differences in the generated schema.

Part of #15662 and #11838

Note that currently the ASP.NET 3.0 templates still contain migrations generated by 2.2. See dotnet/aspnetcore#12168
@ajcvickers ajcvickers mentioned this issue Jul 14, 2019
Merged
ajcvickers added a commit that referenced this issue Jul 15, 2019
@ajcvickers
Add more MusicStore and Identity tests
865b861 
Covering all the queries MusicStore does, and also updating the tests to properly use store-generated values.

Also, added the ModelSnapshots generated by the project templates for 2.1, 2.2, and 3.0, for SQL Server and SQLite. They all diff correctly against the current code showing no differences in the generated schema.

Part of #15662 and #11838

Note that currently the ASP.NET 3.0 templates still contain migrations generated by 2.2. See dotnet/aspnetcore#12168
ajcvickers added a commit that referenced this issue Jul 15, 2019
@ajcvickers
Add more MusicStore and Identity tests
dbf76eb 
Covering all the queries MusicStore does, and also updating the tests to properly use store-generated values.

Also, added the ModelSnapshots generated by the project templates for 2.1, 2.2, and 3.0, for SQL Server and SQLite. They all diff correctly against the current code showing no differences in the generated schema.

Part of #15662 and #11838

Note that currently the ASP.NET 3.0 templates still contain migrations generated by 2.2. See dotnet/aspnetcore#12168
@bricelam bricelam mentioned this issue Jul 19, 2019
Merged
@bricelam
Copy link
Contributor Author

(Updated with pass 4 and a bit of pass 5)

@bricelam bricelam assigned smitpatel and unassigned bricelam Jul 19, 2019
ajcvickers added a commit that referenced this issue Jul 23, 2019
@ajcvickers
More API review changes
aa9fa7b 
* Remove indexMap parameter of `IEntityMaterializerSource.CreateMaterializeExpression()`
* Move `IsEFPropertyMethod` to `MethodInfoExtensions`
* Rename `MemberIdentity` file
* Made members private on `ExpressionPrinter`
* Use `IReadOnlyList` instead of `IList` where appropriate
* Added factory methods to `SqlFunctionExpression`
* Seal `public override ExpressionType NodeType => ExpressionType.Extension;`
* Added dependency object for QueryCompilationContext

Part of #15662
@ajcvickers ajcvickers mentioned this issue Jul 23, 2019
Merged
ajcvickers added a commit that referenced this issue Jul 23, 2019
@ajcvickers
More API review changes
83349fe 
* Remove indexMap parameter of `IEntityMaterializerSource.CreateMaterializeExpression()`
* Move `IsEFPropertyMethod` to `MethodInfoExtensions`
* Rename `MemberIdentity` file
* Made members private on `ExpressionPrinter`
* Use `IReadOnlyList` instead of `IList` where appropriate
* Added factory methods to `SqlFunctionExpression`
* Seal `public override ExpressionType NodeType => ExpressionType.Extension;`
* Added dependency object for QueryCompilationContext

Part of #15662
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 23, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview8 Jul 29, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview8, 3.0.0 Nov 11, 2019
@smitpatel smitpatel mentioned this issue Jul 26, 2022
Merged
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Milestone
3.0.0
Development

No branches or pull requests
5 participants
@bricelam @ajcvickers @smitpatel @roji @AndriySvyryd