Skip to content

Refactor to use annotation access pattern - #716

Merged
austindrenski merged 2 commits into
npgsql:devfrom
austindrenski:refactor-annotation-patterns
Nov 24, 2018
Merged

Refactor to use annotation access pattern#716
austindrenski merged 2 commits into
npgsql:devfrom
austindrenski:refactor-annotation-patterns

Conversation

@austindrenski

Copy link
Copy Markdown
Contributor

I realized while working on #685 that PostgresExtension, PostgresEnum, and PostgresRange broke with (what seems like) the standard annotation access patterns used throughout EF Core.

@austindrenskiaustindrenski self-assigned this Nov 21, 2018
@austindrenski
austindrenskiforce-pushed the refactor-annotation-patterns branch 2 times, most recently from e78127d to a9533caCompareNovember 21, 2018 07:53
Comment threadsrc/EFCore.PG/Metadata/NpgsqlRelationalAnnotations.cs Outdated

@rojiroji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The overall direction is good and makes the good more clean/idiomatic. But please see notes about where the annotations belong.

Comment threadsrc/EFCore.PG/Metadata/NpgsqlRelationalAnnotations.cs Outdated
Comment threadsrc/EFCore.PG/Metadata/INpgsqlRelationalAnnotations.cs Outdated
Comment threadsrc/EFCore.PG/Extensions/NpgsqlMetadataExtensions.cs Outdated
@austindrenski
austindrenskiforce-pushed the refactor-annotation-patterns branch from b17fbfd to e290f96CompareNovember 23, 2018 06:19
@austindrenski
austindrenskiforce-pushed the refactor-annotation-patterns branch from e290f96 to cbfe6e9CompareNovember 23, 2018 06:38
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@roji I took another pass at this with better naming and more specific extension methods (i.e. not on IAnnotatable).

I think it's a step in the right direction, but may still need some refinement.

@rojiroji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks close to ready, see my comment about the interfaces.

For the record I don't think there's huge value in this - it does make for a slightly nicer interface to the annotation data, but lots of code is added to support this...

Comment threadsrc/EFCore.PG/Metadata/INpgsqlAlterDatabaseOperationAnnotations.cs Outdated
@austindrenski
austindrenskiforce-pushed the refactor-annotation-patterns branch from e21fe78 to 1101084CompareNovember 24, 2018 16:28
@austindrenski
austindrenski merged commit b56c93b into npgsql:devNov 24, 2018
@austindrenski
austindrenski deleted the refactor-annotation-patterns branch November 24, 2018 16:48
@rojiroji added cleanup and removed refactor labels May 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@austindrenski@roji@YohDeadfall