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

SQL beautify - all lines are 'beautified' into one long line #63

Closed
jamhall opened this issue Aug 13, 2014 · 29 comments · Fixed by #67
Closed

SQL beautify - all lines are 'beautified' into one long line #63

jamhall opened this issue Aug 13, 2014 · 29 comments · Fixed by #67
Assignees

Comments

@jamhall
Copy link
Contributor

jamhall commented Aug 13, 2014

Hi there,

Nice plugin :-)

I have a little issue with formatting SQL.

Let's take this example:

INSERT INTO client (host, description, created_at) VALUES('hallpclnx', 'My linux machine', CURRENT_TIMESTAMP);
INSERT INTO thread (thread, description, created_at, client_id) VALUES(1, 'Living room camera', CURRENT_TIMESTAMP, 1);
INSERT INTO thread (thread, description, created_at, client_id) VALUES(2, 'Porch camera', CURRENT_TIMESTAMP, 1);
INSERT INTO thread (thread, description, created_at, client_id) VALUES(2, 'Garden camera', CURRENT_TIMESTAMP, 1);
INSERT INTO client (host, description, created_at) VALUES('shedpclnx', 'My shed linux machine', CURRENT_TIMESTAMP);

It gets beautified into this:

INSERT INTO client (host, description, created_at) VALUES('hallpclnx', 'My linux machine', CURRENT_TIMESTAMP); INSERT INTO thread (thread, description, created_at, client_id) VALUES(1, 'Living room camera', CURRENT_TIMESTAMP, 1); INSERT INTO thread (thread, description, created_at, client_id) VALUES(2, 'Porch camera', CURRENT_TIMESTAMP, 1); INSERT INTO thread (thread, description, created_at, client_id) VALUES(2, 'Garden camera', CURRENT_TIMESTAMP, 1); INSERT INTO client (host, description, created_at) VALUES('shedpclnx', 'My shed linux machine', CURRENT_TIMESTAMP); 

I'm not sure about you, but that's not very 'beautiful' ;-)

Perhaps this is standard behaviour? Or I'm missing a config option?

Thanks in advance.

@Glavin001 Glavin001 added the bug label Aug 13, 2014
@Glavin001
Copy link
Owner

Thank you!

Not very pretty at all! Could you check out https://github.com/vkiryukhin/pretty-data and try your code on their website. PrettyData is what I use for SQL beautification. Will have to take a closer look later today when I have a chance.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 13, 2014

Currently doing that right now. Will report back.

@Glavin001
Copy link
Owner

Excellent, thank you.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 13, 2014

That's a shame. pretty-data doesn't handle INSERT statements.

@Glavin001
Copy link
Owner

That is a shame. Is there an issue in progress with pretty-data?

If you can find another SQL beautifier that works better/well for you, please let me know, and I can potentially switch it out :).

@Glavin001
Copy link
Owner

http://sqlformat.org/ appears to work well with your afflicting sample SQL source code.
It is available on GitHub for local installation: http://sqlformat.org/source/
If you could try and set that up and test the Command-Line Interface ``sqlparse.format` method (see https://sqlparse.readthedocs.org/en/latest/intro/#getting-started ), which is something I could support, then I could make an issue for Atom Beautify to switch over 👍. Let me know if this works for you and/or if you find another beautifier .

@jamhall
Copy link
Contributor Author

jamhall commented Aug 13, 2014

So, an update :-)

In your previous comment you striked out 'Command line interface'. There is a command line interface when you install it (sqlformat)

I installed sqlparse and then modified the sql-beautify script:

/**
 * Requires https://github.com/andialbrecht/sqlparse
 */
'use strict';

var cliBeautify = require('./cli-beautify');

function getCmd(inputPath, outputPath, options) {
    return "sqlformat " + inputPath + " --reindent --indent_width=2 --keywords=upper --identifiers=lower"
}

var isStdout = true;
module.exports = cliBeautify(getCmd, isStdout);

That works 👍

The indent_char option is now obsolete. I'll keep indent_size.

The new options to be added:
reindent(true, false), keywords(upper,lower,capitalize. Default: upper?), identifiers(upper,lower,capitalize. Default: lower?)

I'll commit my changes and create a pull request if you're in agreement.

Let me know your thoughts.

Thanks!

@Glavin001
Copy link
Owner

Awesome!!! That is excellent news. Great work. Be sure to use the options being passed in your command (such as the indent_size, which is really the big one). Besides that your code looks good to me.

@Glavin001
Copy link
Owner

Also be sure to use quotes around the input path, for those uses with spaces in paths -- myself included. I keep spaces in my main projects directory so I can test edge cases like that.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 13, 2014

No worries. I'll keep you updated. Cheers.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 13, 2014

Arggh. So I've started atom in development mode (atom -d). Everytime I try to 'beautify' a file the editor just hangs :-( I can't seem to work out why. This is for every type of file.... any idea?

@Glavin001
Copy link
Owner

No idea. Worked fine for me last I tried. Anything in the Developer Tools console? I guess you wouldn't be able to see it if everything is frozen.

@Glavin001
Copy link
Owner

Any luck, @Flukey? Feel free to start the Pull Request early once you have some code in the works so we can discuss and/or if you need a hand 😃. Thank you for contributing!

@Glavin001
Copy link
Owner

@Flukey I can confirm the same issue is happening to me when in Development Mode.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 15, 2014

@Glavin001 Hey. Sorry for the slow reply. It's a bank holiday here in France. Ah, good to know it's not just me. Any idea how I can test my code when not in development mode? I've done the necessary changes to the coffeescript files.

@Glavin001
Copy link
Owner

Just not load in development mode and open up the Developer Tools in normal mode, if necessary. Until we have an interface (See #56 ) we don't really need to use the Development Mode often.

@Glavin001
Copy link
Owner

Now it is also freezing in normal, non-Development-Mode... I'll take another look at the source code. This may be because of my shift into CoffeeScript and the transpiler could have made a mistake (See #60).

@jamhall
Copy link
Contributor Author

jamhall commented Aug 15, 2014

@Glavin001 I can confirm, I am also having the same problem in non-development mode. Perhaps a callback isn't being fired?

@Glavin001
Copy link
Owner

Perhaps, although then I would think it would just not properly beautify (never pass the beautified text to the editor), however instead it is locking up in this strange manner. I'll have to dig into it later: I'm working at the moment. I'll get back to you ASAP. Feel free to submit your PR and I can merge and solve this freezing issue on it's own before I actually publish.

@Glavin001
Copy link
Owner

@Glavin001
Copy link
Owner

Actually, it does end up calling the beautifyComplete callback and successfully reaches: https://github.com/Glavin001/atom-beautify/blob/master/lib/beautify.coffee#L157

I think this could be a breaking change by Atom's editor and it's freezing when we try to set the text of the editor to the new text.

@Glavin001
Copy link
Owner

Confirmed that is it the call to setCursors from here: https://github.com/Glavin001/atom-beautify/blob/master/lib/beautify.coffee#L165

I commented out the setCursors function and it worked as usually. Now I'll try and fix it.

@Glavin001
Copy link
Owner

Yup, http://js2coffee.org/ messed it up 😉.

See

function setCursors(editor, posArray) {
for (var idx = 0; idx < posArray.length; idx++) {
var bufferPosition = posArray[idx];
if (idx === 0) {
editor.setCursorBufferPosition(bufferPosition);
continue;
}
editor.addCursorAtBufferPosition(bufferPosition);
}
}

unction setCursors(editor, posArray) {
  for (var idx = 0; idx < posArray.length; idx++) {
    var bufferPosition = posArray[idx];
    if (idx === 0) {
      editor.setCursorBufferPosition(bufferPosition);
      continue;
    }
    editor.addCursorAtBufferPosition(bufferPosition);
  }
}

Compared to now at https://github.com/Glavin001/atom-beautify/blob/master/lib/beautify.coffee#L25-L35

setCursors = (editor, posArray) ->
  idx = 0

  while idx < posArray.length
    bufferPosition = posArray[idx]
    if idx is 0
      editor.setCursorBufferPosition bufferPosition
      continue
    editor.addCursorAtBufferPosition bufferPosition
    idx++
  return

Notice how if idx is 0 is never reaches the idx++... Wow.

@Glavin001
Copy link
Owner

@Flukey you're good to pull and merge in master. Just pushed fix for #60.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 15, 2014

Excellent. Great work! Thanks for your prompt fix :) I'll try and make the pull request tomorrow. Well, now we know not to trust js2coffee 100% ;-) (incidentally, i used the same tool to convert my js code for this plugin. I'm gonna go over it again now to make sure there are no problems such as this! heh)

@Glavin001
Copy link
Owner

If you submit the pull request we can have a discussion on the PR issue without merging it until we are done. I can help test it out, too.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 15, 2014

Ok. No problem. BTW, do you have access to a windows machine so you can test it? I only have access to linux and mac.

@Glavin001
Copy link
Owner

I have a Virtual Machine of Windows 8 that I can boot up to test.

@jamhall
Copy link
Contributor Author

jamhall commented Aug 15, 2014

Excellent. I'll keep you updated tomorrow. I'm surprised this extension doesn't have more stars, It's very useful! Most of the plugins that offer the same functionality only do it for one language.

jamhall added a commit to jamhall/atom-beautify that referenced this issue Aug 17, 2014
…indent_size(default: 2), sql_identifiers(default: lower. Options: lower, upper, capitalize). sql_sqlformat_path (path to sqlformat command if not on path). This resolves Glavin001#63
@Glavin001 Glavin001 self-assigned this Aug 30, 2014
Glavin001 added a commit that referenced this issue Aug 30, 2014
Fixes #63. Replacing node-parse with sqlparse (python dependency).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants