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

Escape column names #167

Merged
merged 1 commit into from
Sep 15, 2014
Merged

Escape column names #167

merged 1 commit into from
Sep 15, 2014

Conversation

jpmarques
Copy link
Contributor

No description provided.

mgravell added a commit that referenced this pull request Sep 15, 2014
@mgravell mgravell merged commit 3840a2e into DapperLib:master Sep 15, 2014
@mgravell
Copy link
Member

Thanks; much appreciated

@tms
Copy link
Contributor

tms commented Sep 15, 2014

Noooo! :( This regresses (the spirit of) #68, would it be okay to parameterize the escape character/process somehow? I'll go ahead and do that if so.

@mgravell
Copy link
Member

@tms well, damn; yeah, that's a thing. I'll back this out while I think about options

@jpmarques
Copy link
Contributor Author

To better support multi-DB I'd would suggest adding several strategies for each DB dialect and a way of parameterize which one to use, is this over engineering?

For a more simple solution, parameterize the process should do it, maybe by overriding some method?

@mgravell
Copy link
Member

Yes, given the existence of SqlCompactDatabase, this very much feels like a
missing virtual method or two (escape column, escape table). Then the base
version could just return GIGO - SqlDatabase could use square brackets,
etc. Thoughts?
On 15 Sep 2014 21:43, "João Pedro Marques" [email protected] wrote:

To better support multi-DB I'd would suggest adding several strategies for
each DB dialect and a way of parameterize which one to use, is this over
engineering?

For a more simple solution, parameterize the process should do it, maybe
by overriding some method?


Reply to this email directly or view it on GitHub
#167 (comment)
.

@devlead
Copy link
Contributor

devlead commented Sep 15, 2014

PostgreSQL doesn't use braces but quotes, so don't generalize too much.

Sent from my Windows Phone


From: Marc Gravellmailto:[email protected]
Sent: ‎9/‎15/‎2014 22:49
To: StackExchange/dapper-dot-netmailto:[email protected]
Subject: Re: [dapper-dot-net] Escape column names (#167)

Yes, given the existence of SqlCompactDatabase, this very much feels like a
missing virtual method or two (escape column, escape table). Then the base
version could just return GIGO - SqlDatabase could use square brackets,
etc. Thoughts?
On 15 Sep 2014 21:43, "João Pedro Marques" [email protected] wrote:

To better support multi-DB I'd would suggest adding several strategies for
each DB dialect and a way of parameterize which one to use, is this over
engineering?

For a more simple solution, parameterize the process should do it, maybe
by overriding some method?


Reply to this email directly or view it on GitHub
#167 (comment)
.


Reply to this email directly or view it on GitHub:
#167 (comment)

@jpmarques
Copy link
Contributor Author

@mgravell how about adding these four for now?

string PrefixNoCount(string sql)
string AppendIdentitySelect(string sql)
string QuoteIdentifier(string name)
string UnQuoteIdentifier(string name)

Supporting new databases would be a matter of overriding these instead of overriding, for example, the insert method.

@mgravell
Copy link
Member

That's probably a reasonable starting point. What purpose does
unquoteidentifier serve here? Vexingly, DbCommandBuilder already supports
identifiers etc, but is not readily available or well supported :(
On 16 Sep 2014 17:47, "João Pedro Marques" [email protected] wrote:

@mgravell https://github.com/mgravell how about adding these four for
now?

string PrefixNoCount(string sql)
string AppendIdentitySelect(string sql)
string QuoteIdentifier(string name)
string UnQuoteIdentifier(string name)

Supporting new databases would be a matter of overriding these instead of
overriding, for example, the insert method.


Reply to this email directly or view it on GitHub
#167 (comment)
.

@jpmarques
Copy link
Contributor Author

Yes, I think unquoteidentifier can be put aside, no real use for it.

João Pedro Marques

On Tue, Sep 16, 2014 at 8:55 PM, Marc Gravell [email protected]
wrote:

That's probably a reasonable starting point. What purpose does
unquoteidentifier serve here? Vexingly, DbCommandBuilder already supports
identifiers etc, but is not readily available or well supported :(
On 16 Sep 2014 17:47, "João Pedro Marques" [email protected]
wrote:

@mgravell https://github.com/mgravell how about adding these four for
now?

string PrefixNoCount(string sql)
string AppendIdentitySelect(string sql)
string QuoteIdentifier(string name)
string UnQuoteIdentifier(string name)

Supporting new databases would be a matter of overriding these instead
of
overriding, for example, the insert method.


Reply to this email directly or view it on GitHub
<
https://github.com/StackExchange/dapper-dot-net/pull/167#issuecomment-55773405>

.


Reply to this email directly or view it on GitHub
#167 (comment)
.

@jpmarques
Copy link
Contributor Author

What do you think about this changes?

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