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

Drop DqlSelection and Dql namespace #256

Merged
merged 1 commit into from
Mar 15, 2016
Merged

Drop DqlSelection and Dql namespace #256

merged 1 commit into from
Mar 15, 2016

Conversation

foxycode
Copy link
Contributor

Related to #238

@foxycode
Copy link
Contributor Author

@fprochazka This one should be finished too.

@foxycode
Copy link
Contributor Author

@fprochazka Rebased from master

/**
* @author Filip Procházka <[email protected]>
*/
class InlineParamsBuilder extends Kdyby\Doctrine\QueryBuilder
Copy link
Member

Choose a reason for hiding this comment

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

do not remove this one please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fprochazka You wanted to remove whole Dql namespace. Where should I move it then?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I know, but I forgot about this one. Just leave it as is for now please.

@foxycode
Copy link
Contributor Author

@fprochazka Updated as you wanted. I renamed InlineParamsBuilder to InlineParamsQueryBuilder to reflect test name, which seems more proper than renaming test. Hope it's ok.

@fprochazka
Copy link
Member

@foxycode well, how many people are extending tests and how many people are extending classes that are part of a library? I don't think renaming the class is a good idea at this point.

@fprochazka
Copy link
Member

I do agree that is probably a better name, but I don't wanna think about it now and introduce more BC Breaks than neccesary.

@foxycode
Copy link
Contributor Author

@fprochazka Ok, should I rename test then or leave it as is?

@fprochazka
Copy link
Member

Also, by renaming the class, you're combining two unrelated changes in one PR.

@fprochazka
Copy link
Member

Please revert the rename. It is not neccesary right now nor related.

@foxycode
Copy link
Contributor Author

@fprochazka Done

fprochazka added a commit that referenced this pull request Mar 15, 2016
Drop DqlSelection and Dql namespace
@fprochazka fprochazka merged commit cc4a3e8 into Kdyby:master Mar 15, 2016
@fprochazka
Copy link
Member

@foxycode thank you! :)

@fprochazka
Copy link
Member

Again: if anybody was using it, you're welcome to create a new package with this (or better) code that implements it.

@foxycode foxycode deleted the drop-dql branch March 15, 2016 20:40
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.

2 participants