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

feat(sql): add sql formatter #166

Merged
merged 3 commits into from
Dec 2, 2017

Conversation

baptistemesta
Copy link
Contributor

@baptistemesta baptistemesta commented Nov 19, 2017

Hi as discussed in #156 I started to implement the sql formatter using code from DBeaver.

I started from your PR #163 and basically copied code from
https://github.com/serge-rider/dbeaver/blob/af7c4eb421a5e98c7da38f5f2b70f90933a5812d/plugins/org.jkiss.dbeaver.model/src/org/jkiss/dbeaver/model/sql/format/tokenized/SQLTokenizedFormatter.java

The current state is pretty messy. I started to cleanup what is not useful from the original implementation.
There is one test that work ( com.diffplug.spotless.sql.SQLFormatterStepTest#behavior)

I have a few question to continue this work:

  • What kind of test should I write?
  • Do you know were I can find some exotic queries?
  • some thins can be configured (see com.diffplug.spotless.sql.SQLSyntaxManager and com.diffplug.spotless.sql.SQLFormatterConfiguration) what should be configurable in your opinion?
  • license-wise, what should be the license on the files I copied ( jkiss, DiffPlug or both ) ?

Thanks in advance.

return value == null || value.isEmpty();
}

public static boolean equalObjects(Object o1, Object o2) {
Copy link
Member

Choose a reason for hiding this comment

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

Objects.equals would be fine but I think that was introduced in Java 1.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As said in the description. the code is really messy, I will clean it. I first need some help on the few points I mentioned above.

@nedtwigg
Copy link
Member

Great, thanks!

What kind of test should I write?

There's a checklist here: https://github.com/diffplug/spotless/blob/master/CONTRIBUTING.md#how-to-add-a-new-formatterstep

Do you know were I can find some exotic queries?

It's not exotic, but fairly large: https://github.com/mytake/mytake/blob/master/server/src/main/resources/db/migration/V1__initial.sql

Some things can be configured

The key is for the configuration to be serializable, so that up-to-date checking works. I'm fine if it just has the config that you need - if other people need more, they can add it in future PR's.

license-wise, what should be the license on the files I copied

Definitely jkiss. The DiffPlug copyright will be at the top, but the javadoc for the code should be clear that the real copyright owner is jkiss.

@baptistemesta
Copy link
Contributor Author

I updated the PR according to your feedbacks. I might still missing some copyright.

Let me know if it needs some change.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This is fantastic, thanks so much. My only feedback is on the name of the step and extension method, I added comments inline.

/** Wraps up [BasicFormatterImpl](https://docs.jboss.org/hibernate/orm/4.1/javadocs/org/hibernate/engine/jdbc/internal/BasicFormatterImpl.html) as a FormatterStep. */
public class SQLFormatterStep {

static final String NAME = "defaultSql";
Copy link
Member

Choose a reason for hiding this comment

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

How about something that identifies its source, like dbeaverSql?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure yep. I'll rename everything to Include DBeaver in names

}

public SQLFormatterConfig sqlFormatter() {
return new SQLFormatterConfig();
Copy link
Member

Choose a reason for hiding this comment

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

The name here should probably match the step name. So if we name the step dbeaverSql, then we should probably name this method dbeaver. We might add other sql formatters later, so its best to name them fairly specifically. (we've got a couple different java formatters right now, and we might have a few different Kotlin formatters soon too).

Copy link
Member

@nedtwigg nedtwigg Nov 21, 2017

Choose a reason for hiding this comment

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

Oh, also run spotlessApply ;-) (see CI)

@nedtwigg nedtwigg mentioned this pull request Nov 21, 2017
This first version of the sql formatter is taken from dbeaver project.

It is configurable using closure

```
sql {
   dbeaver().configFile 'sqlFormatter.properties
}
```

configuration file support following properties (with default values)

```
sql.formatter.keyword.case=UPPER
sql.formatter.statement.delimiter=;
sql.formatter.indent.type=space
sql.formatter.indent.size=4
```
@nedtwigg
Copy link
Member

nedtwigg commented Dec 1, 2017

Sorry @baptistemesta, this is sat stagnant for too long. I'll make sure it's merged and released by end-of-day.

@nedtwigg
Copy link
Member

nedtwigg commented Dec 1, 2017

Would you like any changes in how your work is credited or documented? If it looks good to you, I'll merge and release.

@baptistemesta
Copy link
Contributor Author

LGTM like that :) 👍

@nedtwigg nedtwigg merged commit 0b54310 into diffplug:master Dec 2, 2017
@nedtwigg
Copy link
Member

nedtwigg commented Dec 2, 2017

Published in 3.7.0.

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.

3 participants