The future of prettier-sql / sql-formatter about prettier-sql HOT 28 CLOSED

Now also noticed that you reached out earlier about this fork: sql-formatter-org#175
Sorry for not responding then.

from prettier-sql.

Now that I've looked around more, I see some areas in need of improvement:

• Test coverage has dropped to 93%. Notably the tests for comma formatting are not enabled, and when I try to run them, they fail.
• Uppercasing of keywords is now the default behavior, and preserving the case is no more possible. I think it's a serious regression. Initially sql-formatter didn't even have support for uppercasing of keywords, but I reluctantly finally added it. I think by default a code formatter should only make changes to whitespace. Changing non-whitespace characters always bares more risk of changing the behavior of the code.
• The same goes for the new aliasAs option - there's no way to preserve the original style, and the default is a strange add AS to select, but remove from table aliases (I've never would have expected such a style).
• There's a lack of documentation for all the new configuration options. One can find it in code, but even there it's lacking. e.g. what does breakBeforeBooleanOperator do? The comment simply restates the name. The newline option is the most confusing to me - even after playing around with the online demo I can't really make sense of it.
• I'm not really fond of the switch to tabs for indentation. I wouldn't get into this religious topic in an ordinary codebase, but here we have a lot of tests that actually test indentation, and these tests use spaces, while the code surrounding them uses tabs - as a result it's now a sometimes-spaces-sometimes-tabs indentation, which is just inconsistent. It also makes it harder to compare the new code here with old code in sql-formatter, as all lines have changed :(
• The Formatter.ts file has grown from 250 to 700 lines :(
• There's no real Git history of the changes made - just one huge commit for each version :(

But I think overall there are many more positive changes which I applaud:

• I see lots of work gone into gathering all the various reserved names in various SQL dialects.
• The keyword categories (toplevel, newline, toplevel-noindent) have been replaced with more semantic ones (command, binary command, dependent clause, logical operators).
• All the new options for configuring the formatting.
• I appreciate the additional comments which aid my understanding of all the new code.
• Switch to TypeScript is definitely a major step forward in maintainability.
• Last, but not least, you have fixed various issues reported in the sql-formatter repository.

Regarding the latter, I also have a question:

• Have you pulled in any code from the various pull requests in sql-formatter repository (either directly or indirectly) or is all the code here fully by you?

from prettier-sql.

Having looked through all the new config options, I think they need a major overhaul.

For example these all relate to placement of newlines:

• newline
• breakBeforeBooleanOperator
• parenOptions.openParenNewline & .closeParenNewline
• linesBetweenQueries
• semicolonNewline
• lineWidth

Mostly the term "newline" is used, but occasionally also "break" and just "line". Should consistently always use "newline". The generic-sounding newline option should be renamed to something more specific - it only controls how newlines are added to list of clauses.

These options all deal with tabular formats:

• keywordPosition: "standard" | "tenSpaceLeft" | "tenSpaceRight"
• tabulateAlias: boolean
• commaPosition: "before" | "after" | "tabular"

The "ten space" terminology confuses me. I think it would be better to call them all tabular. If I understand correctly, then using keywordPosition: tenSpace* will render the general indent option irrelevant. Also, looks like parenOptions don't work when tenSpace keywordPosition is used.

Mostly just thinking out loud here...

from prettier-sql.

So... my current plan is as follows:

1. Merge all the code from this repo back to sql-formatter, and while doing so, merge both the large commits from master branch and the actual smaller commits from all the v5.X.X release branches. I'd really like to preserve all the git history for future reference.
2. Rename it back to sql-formatter.
3. Maybe release it as sql-formatter 5.1.1. Not really sure though... it's not really backwards-compatible with sql-formatter. Even when one adjusts to the new API, there are these regressions in handling of keyword case and the addition/removal of AS keywords. I think it might be better to skip the sql-formatter 5.x release and go straight to 6.x.
4. Fix the problems with uppercase and aliasAs options.
5. Improve naming of all these new config options.
6. Write some proper docs for the config options.
7. Release it as sql-formatter 6.

from prettier-sql.

I quickly ran into a small problem with the first step above. There is no branch for the 5.1.1 release, so I can't really check out the PR #81 from command line.

So, if you could restore the v5.1.1 branch, it would be of tremendous help.

Nevermind... figured out how to access it.

from prettier-sql.

Pushed the branch that resulted from all this merging in here: https://github.com/zeroturnaround/sql-formatter/tree/prettier-sql

from prettier-sql.

Hi Rene, thanks for getting back to me - meant to respond to this yesterday but got busy.
Yes, I'd be willing to become head maintainer for sql-formatter/prettier-sql, not sure how exactly to resolve git repos/npm though.
But I will be on hiatus for another month or so as I'm currently focusing on another project.

To address your concerns:
Test coverage has dropped to 93%

• did notice this as an item to work on but was initially more focused on getting features in / bugs out, would love to get this up

Uppercasing of keywords is now the default behavior
The same goes for the new aliasAs option

• I will admit I'm a bit more opinionated on the uppercase option but I agree in that a formatter should perform whitespace changes as a baseline so I'll add an option that allows users to choose if they want more changes beyond just whitespace

I'm not really fond of the switch to tabs for indentation

• This was definitely my workspace formatter blanket applying for all files, I don't have a strong preference but I do agree that it does make diffing more obtuse - would gladly revert for clarity's sake

There's no real Git history of the changes made

• the git history is mostly in each of the release branches, with changes summarised in the CHANGELOG - which are then all squashed into master. this was a new branching strategy that I'm rethinking

Have you pulled in any code from the various pull requests in sql-formatter repository (either directly or indirectly) or is all the code here fully by you?

• there aren't any official merges of PRs from sql-formatter but I did write my own implementations / alter existing approaches in existing PRs (you can look through the PR history, all the relevant issues are linked)

Mostly the term "newline" is used, but occasionally also "break" and just "line". Should consistently always use "newline"

• I agree in that these config names should be more consistent, will address in an update in near future

The "ten space" terminology confuses me. I think it would be better to call them all tabular.

• the "ten space" mode is not quite the same as tabular since it does have some elements of the standard cascading approach. I was struggling to find a proper name for this style since it's not one I use myself but I have seen it around, other name ideas were along the lines of "center column" or "column aligned" but open to suggestions here

merge both the large commits from master branch and the actual smaller commits from all the v5.X.X release branches

• the squash commits in master contain all the changes for each release branch. If you wanted to merge the changes and preserve all git history, you can just merge each of the release branches (5.0.0, 5.0.1, 5.1.0, 5.1.1)

from prettier-sql.

I think right now is probably the best time to perform this hand off as the next version release I am planning will completely overhaul the formatting system (3 step system using moo for query lexing, nearley for AST/CST parsing, and then splitting Formatter.ts into multiple component files and using that logic as the final format step).

I've started development on the first step using moo but currently have it on hold as I'm on hiatus here.

Lastly, I also had plans to release this library as a plugin for prettier (hence the name) but don't exactly have a timeline for that

from prettier-sql.

Thanks for the response.

I will be on hiatus for another month or so as I'm currently focusing on another project.

Not a problem at all. I guess I'm currently on a hiatus as well... just haven't thought of calling it so :)

I'm currently in the mood of working on this project again for at least a while. So I'll be looking into fixing these problems that I pointed out. I've already managed to fix most of the disabled tests.

I am planning will completely overhaul the formatting system (3 step system using moo for query lexing, nearley for AST/CST parsing).

Yeah, the current approach is really one big hack, though I have no idea how much effort it will be to create parsers for all these SQL languages. But surely, once that hurdle has been overcome, the formatting would be much simpler with an actual AST at hand.

from prettier-sql.

I see you've merged the present changes (+ some) into master of sql-formatter, is your current plan to release a v6 with the present name and configs?

from prettier-sql.

Yeah, that's generally the plan. I'm still pondering over the config options. I've already renamed several of them, and thinking of renaming more. But I'm also questioning the actual need for some of these. I've managed to trace the origin of some:

• newlineBeforeSemicolon (previously semicolonNewline) - sql-formatter-org#134
• newline: <number> - sql-formatter-org#168

Several other new options look mostly sensible.

But some of the new formatting styles look really odd to me. I've never seen anybody using them:

• keywordPosition: tenSpaceLeft | tenSpaceRight - never would have thought of formatting SQL code like so.
• commaPosition: before | tabular - I've seen this sort of comma-first style in JavaScript, but never in SQL, though I guess the idea is the same. The tabular comma placement just seems terrible from maintenance perspective.
• tabulateAlias - this seems the most sensible out of these, though the alignment will get pretty silly when there's an odd long expression before the alias.

There has been a feature request for tabulated CREATE TABLE statements, which I've actually seen used in practice.

I'd like to know what motivated you to implement these specific formatting styles. Even better would be to have some examples of codebases that use these styles in practice.

from prettier-sql.

Found one example on my own. Googled for some SQL syntax and found a Stackoverflow question that uses the tenSpaceRight formatting style. However it's not 10 spaces, it's an entire 12 spaces. https://stackoverflow.com/questions/5325033/plsql-insert-into-with-subquery-and-returning-clause

from prettier-sql.

I've definitely seen commaPosition: before used before, some pros users say about it is that it allows for easier line commenting + you don't have to find the comma at the end of the line.
commaPosition: tabular is somewhat of a complement to tabulateAlias (but is also part of tabular table statements)

newlineBeforeSemicolon, newline: number, and tabular table creates are driven from my own usage/ seen at my workplace.

The tenSpace formats are the ones I'm least familiar with as I've only seen them used on StackOverflow (would love to takea survey) but the 10 spaces is just an arbitrary width that I decided on based on the length of standard SQL keywords

from prettier-sql.

Sure, we can just stay here - I was just mostly looking for a space where you can help me get set up with maintainer rights for sql-formatter / setup other repo items (ie. Github Actions, npm, etc.)

Additionally, I'd like to go over git hygiene and branching + merging strategies to avoid excessive direct commits on protected branches (ie. master)

from prettier-sql.

Well, at the moment I'm not really in hurry with all that. First, it's not just a click-of-a-switch from my part as I don't actually have full rights to manage ownership of sql-formatter - I'll need to reach out to some people in Zeroturnaround to get this done. Second, having spent some time with the new code, I'm not really that happy with the quality of the changes made in this fork, so I'd like to keep a bit closer eye over the code going in, at least for start.

Sorry, if this comes across as harsh. Please don't feel discouraged. I really do appreciate the effort you've put in here. It's way above the usual quality of contributions I get in most open source projects. My standards just tend to be ridiculously high.

I understood that currently you're tied up in another projects and don't have time to contribute much, so it shouldn't really hinder you in practical terms. If that changes, I suggest you first do some pull requests, and I'll give you some feedback, and when that seems to be working well, I'll get you direct push and maintainer rights. How does this sound?

PS. It's actually not the first time I'm merging back a fork of sql-formatter. Previously there have also been sql-formatter-plus and @gwax/sql-formatter. This one is definitely the largest though.

from prettier-sql.

Regarding your concern about branching+merging...

I really don't see much of an issue with committing directly to master when there's just 1..2 developers on a project. And at the moment it's really just me and my goal is to move fast (but not break things :) ) and not hinder myself with any unnecessary processes. Especially as I'm doing changes all over the place, and not really working on one specific feature.

For me master is really just the branch where active development happens. I wouldn't really require anything beyond "tests should be green" in it.

from prettier-sql.

After the initial PR I made went neglected, I'd assumed the upstream was abandoned so this project transitioned into a new project, effectively with the intention of never merging back to the upstream and eventually superseding it entirely so I'm not entirely on board with how you're handling the merge in sql-formatter.

I was under the assumption that you had full ownership rights to the original repo - if that's not the case, why not just migrate the project under this repo and skip the bureaucracy ?

I'd like to push for using this repo as the new starting point (with the better name that ties both the logo I made and the end goal of making this a prettier plugin alongside the main package ) and migrating the issues from the upstream to here.

from prettier-sql.

I agree it would be best to have full ownership of the repo. I should contact them and ask whether they would be on board with completely transferring the ownership of the repository and the npm package.

Regarding the name, I would be happy to rename if it weren't for the case that sql-formatter has such a big number of people using it already. As long as the name stays the same, they can more easily find out about new versions and upgrade.

I propose I reach out to Zeroturnaround and find out what's the situation with this.

from prettier-sql.

Also, reconsidering what I wrote earlier, I think I have been acting in overly protective way here. I think you deserve to be an equal contributor with full rights. After all the effort you've put in here, this project should really be yours. After all, I have a pretty lousy track-record of maintaining it (more like neglecting it).

I think it would also be good to make a video call (zoom, hangouts, ...). These text-based discussions are way too good at raising unnecessary tensions. Please suggest a suitable time slot. I have no idea in which timezone and country you might live in. I'm in Estonia at UTC+3.

from prettier-sql.

Great success.

I reached out to ZeroTurnaround with my questions and they're on board with transferring the ownership of the repository. It'll be on the week after next week though, as the person handling the repository rights is on vacation.

While investigating this, I also realized that I actually already have full ownership to the npm package. I can easily add you as a maintainer there.

But rather than transferring the ownership of the repo to either your or mine account, I propose creating a separate organization in Github (with the same name as the library) and transferring the ownership under that. This way it all will sit on a more neutral ground and will allow transferring the ownership of the repo again and again in the future (shall the need arise) without it causing a change in the URL.

from prettier-sql.

Thanks for the good update !
I think transferring the repo to a new github org is a great idea and would help to formalise things a bit more.

I would definitely appreciate having you help maintain alongside if you have the bandwidth.
I'll admit that my changes were not always up to par since I didn't have anyone to review my code but had to push them through for the sake of unblocking additional features.
That's also why I'll want to set up a slightly more formal PR structure (at the very least have a develop branch for working changes) to keep each other in check before code gets merged into master.

As for video call scheduling, I live on the US West Coast (UTC + 8) so I think sometime in the morning for me / evening for you would work best.

Thanks a lot for all the support you've given so far and connecting the dots with ZeroTurnaround

from prettier-sql.

morning for me / evening for you would work best.

Yeah. My evenings are booked today and tomorrow, but I should be free on Sunday...Wednesday.

from prettier-sql.

Sounds good, we can do 10:00 PST / 20:00 Estonia time on Monday - do you have a preference on which platform ?

from prettier-sql.

Great. No real platform preference from my side.

from prettier-sql.

I released multiple betas, the latest being v6.0.0-beta.3.

Also updated the demo page to use that latest beta.

At the moment this looks final enough for me. Wrote a draft of release notes: https://github.com/zeroturnaround/sql-formatter/releases/tag/untagged-444e5ddaa3cb6746478f

Unless you see something wrong, I'll release this as 6.0.0 tomorrow.

from prettier-sql.

created a gitter room so we can more easily chat about repo details: https://gitter.im/sql-formatter/admin#

from prettier-sql.

everything in the new release seems to be pretty good, removing LATERAL and THEN from the ReservedDependentClauses lists and reverting the CASE seems iffy to me but can workshop those later so long as they're working now

from prettier-sql.

Closing this in favour of the gitter channel

from prettier-sql.