search

auraphp / Aura.SqlQuery

Independent query builders for MySQL, PostgreSQL, SQLite, and Microsoft SQL Server.
MIT License
452 stars 86 forks source link

3.x quoter #132

Closed pmjones closed 7 years ago

pmjones commented 7 years ago

This PR creates separate quoter classes for each driver (notably mysql and sqlsrv).

@harikt @pavarnos Thoughts?

harikt commented 7 years ago

@pmjones

Thank you for pinging.

I am not sure what advantage do you see in moving to a separate class than passing the string?

pavarnos commented 7 years ago

@harikt it removes db specific config from the generic QueryFactory class https://github.com/auraphp/Aura.SqlQuery/pull/132/files#diff-f4d2c86a7d03f40d9533ddaca828a28fL47 into the db specific driver directory, which is an improvement in clarity. It makes the quoter re-usable too (eg in Aura.Sql?).

@pmjones how is the test coverage? I can't seem to find it...

pmjones commented 7 years ago

@harikt As @pavarnos notes, it's "better form" than before, but not in any way functionally critical.

@pavarnos Test is at https://github.com/auraphp/Aura.SqlQuery/pull/132/files#diff-c13b8f7f14b2d2fbc0552d98128d064f and coverage should be the same as before (i.e. 100%); the db-specific quoters only modify properties, not methods. Cf. https://scrutinizer-ci.com/g/auraphp/Aura.SqlQuery/?branch=3.x

Other thoughts?

harikt commented 7 years ago

@pmjones @pavarnos thank you.

The quoter is actually introduced in https://github.com/auraphp/Aura.Sql/pull/153 .

So I doubt about it.

pavarnos commented 7 years ago

Thanks. Temporary blindness: I cannot find the coverage report / percentage?

pmjones commented 7 years ago

@harikt So is that a "yes" or a "no" on this PR?

@pavarnos At the Scrutinizer link, on the right, under "Badges", it shows 100%.

harikt commented 7 years ago

@pmjones I was answering to @pavarnos

It makes the quoter re-usable too (eg in Aura.Sql?).

The quoter is actually introduced in auraphp/Aura.Sql#153 .

So I doubt about it.

Probably the Quoter in that case can be moved out. But that may break what we advertise, but less things being duplicated.

pmjones commented 7 years ago

@harikt Ah right. Yes, the quoter in Sql is much more limited.

@harikt @pavarnos Just so that we can either merge or close this PR, and I back to the "actual plan" laid out in #108 , give me "go" or "no-go" and I'll work from there.

harikt commented 7 years ago

@pmjones I am :+1: . So "go" .

pavarnos commented 7 years ago

good with me

pmjones commented 7 years ago

Thanks folks. Back to #108 !