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

Kotlin support in Maven plugin #223

Merged
merged 9 commits into from
Mar 13, 2018
Merged

Conversation

lutovich
Copy link
Contributor

No description provided.

Moved all generic formatter methods to `FormatterFactory` so that it is
a base class for every configuration element. Made `Java`, `Scala` and
`Format` extend it.
Kotlin formatter now supports all generic formatting steps
(e.g. license header) and standard Ktlint formatter with
configurable version.
To share same regex between Gradle and Maven plugins.
@lutovich lutovich requested a review from nedtwigg March 12, 2018 22:48
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.

Fantastic! I think the change in responsibility between Format and FormatterFactory is a good choice. Press "merge" whenever you'd like 👍

public class Kotlin extends FormatterFactory {

private static final Set<String> DEFAULT_INCLUDES = unmodifiableSet(newHashSet("src/main/kotlin/**/*.kt",
"src/test/kotlin/**/*.kt"));
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ignore this, but I would've written as ImmutableSet.of("src/main/.... ImmutableSet gives compile-warnings if you accidentally call modifier-methods like put, and it's a little faster since it doesn't have to worry about the modification usecase.

@nedtwigg
Copy link
Member

Just noticed that there's no docs for kotlin or scala: https://github.com/diffplug/spotless/blob/master/plugin-maven/README.md

@lutovich
Copy link
Contributor Author

@nedtwigg updated code to use ImmutableSet and added readme entries for Scala and Kotlin. Thanks for the review!

@lutovich lutovich merged commit 2de1b68 into diffplug:master Mar 13, 2018
@lutovich lutovich deleted the kotlin-fmt-maven branch March 13, 2018 20:21
@@ -32,7 +32,7 @@
import com.diffplug.spotless.Formatter;
import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.LineEnding;
import com.diffplug.spotless.maven.generic.LicenseHeader;
import com.diffplug.spotless.maven.generic.*;
Copy link
Member

Choose a reason for hiding this comment

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

I admit that I was surprised that this change from a full import to a wildcard import was apparently accepted by Travis CI.

@nedtwigg Do you think it would be a good idea if I open an issue to cover catching wildcard imports in the future?

Copy link
Member

Choose a reason for hiding this comment

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

The eclipse formatter that we use doesn't have any rules against wildcard imports. It might be worth a bullet-point in this issue, maybe a config option on importSorter if it ever gets updated: #167

Copy link
Member

Choose a reason for hiding this comment

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

@nedtwigg I'm not sure that I understood your last message correctly.

Did you mean that you'd like me to write a new message on #167 - a reminder of sorts - to add a convert-wildcard-imports-to-full-imports option to the new importOrder() formatting option that would be implemented? Or did you mean something else?

Copy link
Member

Choose a reason for hiding this comment

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

Yup! You can make a new issue too if you'd like, but it seems like implementing the wildcard imports issue is so closely tied to the ImportSorter that it makes sense to bundle them.

Copy link
Member

Choose a reason for hiding this comment

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

Cool! I'll go act on that now, then. :)

@nedtwigg
Copy link
Member

@lutovich Would you like me to cut a release with these changes? If so, would you like it to be another BETA, or should it be 1.0.0? We can ship 1.0.0 without an example opensource project, and delay the marketing until we have an example.

@lutovich
Copy link
Contributor Author

Hi @nedtwigg, sorry for the delayed response. I'd vote for another beta, do not have a direct use-case for the Kotlin formatter.

public final class KotlinConstants {

// '^' is prepended to the regex in LICENSE_HEADER_DELIMITER later in FormatExtension.licenseHeader(String, String)
public static final String LICENSE_HEADER_DELIMITER = "(package |@file)";
Copy link
Member

Choose a reason for hiding this comment

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

👍

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