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

TextHandler handles now ArraySegment<char> #1411

Merged
merged 2 commits into from Jan 24, 2017

Conversation

Projects
None yet
2 participants
@Scooletz
Contributor

Scooletz commented Jan 23, 2017

This PR enhances TextHandler making it possible to pass ArraySegment<char> as a parameter value.

@Scooletz

This comment has been minimized.

Show comment
Hide comment
@Scooletz

Scooletz Jan 23, 2017

Contributor

I've got a question related to this PR: what's your release policy, is it described anywhere?

Contributor

Scooletz commented Jan 23, 2017

I've got a question related to this PR: what's your release policy, is it described anywhere?

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jan 24, 2017

Member

@Scooletz thanks for this, from a quick look it looks good.

One comment - you probably want to add ArraySegment<char> to the type mapping attribute's type inference list at the top (where it already contains string, char[], char) - this will make Npgsql automatically send text for ArraySegment<char> even if NpgsqlDbType.Text isn't specified.

Regarding the release policy, it's a bit fluid but features are generally not released in patch releases unless their risk is very low. For example, I'd be comfortable releasing this PR in a patch-level release because it's very hard to imagine breakage as a result. Minor releases (3.1, 3.2) definitely introduce breakage, so Npgsql follows PostgreSQL more than it does strict semver. BTW, the Npgsql 3.2 is days away. Hope this clarifies things.

Member

roji commented Jan 24, 2017

@Scooletz thanks for this, from a quick look it looks good.

One comment - you probably want to add ArraySegment<char> to the type mapping attribute's type inference list at the top (where it already contains string, char[], char) - this will make Npgsql automatically send text for ArraySegment<char> even if NpgsqlDbType.Text isn't specified.

Regarding the release policy, it's a bit fluid but features are generally not released in patch releases unless their risk is very low. For example, I'd be comfortable releasing this PR in a patch-level release because it's very hard to imagine breakage as a result. Minor releases (3.1, 3.2) definitely introduce breakage, so Npgsql follows PostgreSQL more than it does strict semver. BTW, the Npgsql 3.2 is days away. Hope this clarifies things.

@Scooletz

This comment has been minimized.

Show comment
Hide comment
@Scooletz

Scooletz Jan 24, 2017

Contributor

Cool, I'll add the mapping then. Should I provide a cherry-picked version on top of some branch or could I leave it for you @roji ?

Contributor

Scooletz commented Jan 24, 2017

Cool, I'll add the mapping then. Should I provide a cherry-picked version on top of some branch or could I leave it for you @roji ?

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jan 24, 2017

Member

Leave it as it is, I'll do the necessary changes (the whole type handler system is undergoing a huge overhaul for 3.2 in a branch). But it would be great if you could amend the PR quickly.

Member

roji commented Jan 24, 2017

Leave it as it is, I'll do the necessary changes (the whole type handler system is undergoing a huge overhaul for 3.2 in a branch). But it would be great if you could amend the PR quickly.

@Scooletz

This comment has been minimized.

Show comment
Hide comment
@Scooletz

Scooletz Jan 24, 2017

Contributor

Done @roji

Contributor

Scooletz commented Jan 24, 2017

Done @roji

@roji roji merged commit 6efacbe into npgsql:dev Jan 24, 2017

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jan 24, 2017

Member

@Scooletz thanks, merged for 3.2.

Member

roji commented Jan 24, 2017

@Scooletz thanks, merged for 3.2.

@roji roji requested review from roji and removed request for roji Jan 24, 2017

@roji roji added the enhancement label Jan 24, 2017

@roji roji added this to the 3.2 milestone Jan 24, 2017

@Scooletz Scooletz deleted the Scooletz:array-segments-for-texts branch Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment