Skip to content

Conversation

chregu
Copy link
Contributor

@chregu chregu commented Apr 19, 2012

This adds a generic Qom to SQL1 Convertor like the SQL2 one to be used in jackalope-jackrabbit for letting the the QOM decide which SQL engine should be used

See also:

jackalope/jackalope#115
jackalope/jackalope-jackrabbit#17

@dbu
Copy link
Member

dbu commented Apr 19, 2012

are you sure the converter and generator classes can not share code? its just that the SQL2 thingy is quite buggy and when we duplicate that we have two buggy things...

your last commit says "(more to come)" - is this ready to be merged?

@chregu
Copy link
Contributor Author

chregu commented Apr 19, 2012

I could try and see how many methods are still the same, in the generator it won't be much, maybe in the Converter it's more

@dbu
Copy link
Member

dbu commented Apr 19, 2012

i don't want to be complicated but i fear that if we merge this we won't look at it too soon and the pain will just grow :-(

@chregu
Copy link
Contributor Author

chregu commented Apr 19, 2012

No, you're of course right.

@chregu
Copy link
Contributor Author

chregu commented Apr 19, 2012

I "merged" the 2 generators now (I worked on them anyway). Next task will be the Converter. Should be able to do it tonight

@chregu
Copy link
Contributor Author

chregu commented Apr 19, 2012

tests added, code shared in QueryConverters as well

*/
protected function convertStaticOperand(QOM\StaticOperandInterface $operand)
{
if ($operand instanceof QOM\BindVariableValueInterface)
Copy link
Member

Choose a reason for hiding this comment

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

curlies are a bit off :)

@lsmith77
Copy link
Member

and yes i know .. some of the CS violations where there before the refactoring.

@chregu
Copy link
Contributor Author

chregu commented Apr 19, 2012

hehe, no problem. Thanks for the feedback, will clean it up tomorrow or over the weekend. Too tired right now ;)

@lsmith77
Copy link
Member

cs issues should be fixed

@chregu
Copy link
Contributor Author

chregu commented Apr 19, 2012

wow. thanks

dbu added a commit that referenced this pull request Apr 20, 2012
Adding QomToSQL1Converter
@dbu dbu merged commit 95f4923 into master Apr 20, 2012
@dbu
Copy link
Member

dbu commented Apr 20, 2012

great, thanks a lot!

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.

3 participants