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

Force uppercase SQL keywords #93

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Conversation

rinu
Copy link
Contributor

@rinu rinu commented Aug 16, 2023

I don't see any benefit to anyone ever having lowercase keywords.

Not all reserved words are detected correctly in this project. A simple regex is simple but it lacks context. But so what, if it's a field name, put ticks around it.

Closes #73

@derrabus
Copy link
Member

How does this PR relate to #73?

@rinu
Copy link
Contributor Author

rinu commented Aug 16, 2023

How does this PR relate to #73?

This is much simpler. No options to argue about. If accepted, it would replace #73.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

From #73:

This adds a new optional third parameter to the format method, $forceUppercase which defaults to false to maintain backwards compatibility.

I don't think we should consider changing case as breaking compatibility.

@rinu
Copy link
Contributor Author

rinu commented Aug 16, 2023

I don't see the benefit of keeping compatibility in this case.

@greg0ire
Copy link
Member

I'm not saying we should not keep compatibility, I'm saying that we should consider that we are keeping it: a change in case should IMO be considered compatible.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

a change in case should IMO be considered compatible.

I haven't worked with a SQL dialect yet that was case sensitive.

@greg0ire greg0ire added this to the 1.2.0 milestone Aug 16, 2023
@greg0ire greg0ire merged commit a321d11 into doctrine:1.2.x Aug 16, 2023
9 checks passed
@greg0ire
Copy link
Member

Thanks @rinu !

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.

None yet

4 participants