-
Notifications
You must be signed in to change notification settings - Fork 23
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
Option to force uppercase SQL keywords #73
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
acb59a3
A few quick formatting tweaks
othyn b309a31
Add uppercase auto-formatting as a passable option to the format meth…
othyn abba52b
Fix code standards outlined by CONTRIBUTING.md: $ echo '' | vendor/bi…
othyn 996bd56
Document the uppercase behaviour in the readme
othyn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How likely is it that somebody will want to sometimes uppercase SQL words, sometimes not in the same application? I wonder if this wouldn't be better as a constructor parameter. What do you think @goetas @rodnaph @othyn ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, unlikely - the purpose of the tool is standardisation.
(If someone did, they could create multiple instances perhaps).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too bad I did not use a more generic name when introducing
Highlighter
in #21 , this looks like it could be implemented more cleanly with a custom highlighter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man it's so long ago I can't even remember my own use case! Haha 😅 I think... it was regarding an export tool I was writing to generate formatted SQL statements from Laravel migrations, in which having uppercased keywords suited that functionality better.
As for implementation, if you think it works better as a constructor option then I'm happy to oblige.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That or maybe there should be an abstraction next to
Highlighter
. Highlighting is about adding things before and after, there could be another interface in charge of changing the case, so that you would haveand
echo (new SqlFormatter(new NullHighlighter(), new UppercaseSomethingSomething()))->format($query)
Naming ideas welcome, and let's wait for @goetas to look at this, he's good at design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SenseException can you please rephrase? I don't understand 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$forceUppercase
and also$indentString
are used for the representation of the SQL string output and I see both as options that could be injected as a config for the second constructor argument. While$indentString
has to stay in theformat
function for version 1, I can imagine that a future 2.0 can contain this info for "use uppercase" and "use spaces as indent" inside e.g. a Config object thatSqlFormatter
(or somewhere else) gets injected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a
fromConfig
named constructor that should call a somewhat complicated constructor in v2 yeah. PHP 8 usage could beSqlFormatter::fromConfig(indentString: '>', case_strategy: UPPERCASE_SQL_KEYWORDS, highlighter: HTML)
and then it would result in the constructor being called with the right implementations, that way we don't have to maintain a config object with properties, getters and setters, it all gets validated and thrown away in the named constructor, and people can still swap in / swap out their own implementations by using the normal constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of a named constructor. Not having a Config object to maintain has its advantages, but getter and setter methods aren't needed considering typed properties that are already part in 7.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sustain it, but adding a $forceUppercase option would add only ONE option.
Please read my last suggestion instead : #85