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

tsfmt maven plugin #553

Merged
merged 32 commits into from Apr 2, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ed9612b
Add Typescript Tsfmt to Maven Plugin
source-knights Mar 26, 2020
0e429a5
Update README.md
source-knights Mar 26, 2020
bb72ebd
Update README to include tsfmt maven plugin
source-knights Mar 26, 2020
ae05010
Update CHANGES.md
source-knights Mar 26, 2020
588990a
Update CHANGES.md
source-knights Mar 26, 2020
e973a2e
Fix unit test, correct xml end tag
source-knights Mar 26, 2020
2ebe163
Spotless apply
source-knights Mar 26, 2020
6a71794
Merge branch 'master' of https://github.com/source-knights/spotless
source-knights Mar 26, 2020
588aa43
Spotless apply
source-knights Mar 26, 2020
528e708
Merge branch 'master' into master
nedtwigg Mar 28, 2020
811c661
Make the Typescript defaults a little less expensive to compute, and …
nedtwigg Mar 29, 2020
90a1870
Condense the typescript docs for plugin-maven.
nedtwigg Mar 29, 2020
97beb15
DRY on test files - reveals that tsfmtInline and tsconfig are both no…
nedtwigg Mar 29, 2020
8c10f56
Make it easier to run single maven-plugin tests from the IDE.
nedtwigg Mar 29, 2020
7bd2220
Fix warning about -color
nedtwigg Mar 29, 2020
e9da142
Fix types in the inline config.
nedtwigg Mar 29, 2020
2641254
Fix buildDir going into Tsfmt
source-knights Mar 30, 2020
9ac7a49
Fix tsconfig file
source-knights Mar 30, 2020
33b13f6
spotless apply
source-knights Mar 30, 2020
80cc770
Fix issue with FileLocator and tsconfig file
source-knights Mar 30, 2020
daff537
spotless apply
source-knights Mar 30, 2020
839bcbd
spotless apply
source-knights Mar 30, 2020
56ddfcb
add tsconfig test to plugin gradle
source-knights Mar 30, 2020
5593769
Make the buildDir a non-optional part of FileLocator.
nedtwigg Mar 31, 2020
5f4bbfa
Give FileLocator an in-place locator capability.
nedtwigg Mar 31, 2020
6d21884
Make the gradle tests use the same test files as the maven and lib te…
nedtwigg Mar 31, 2020
9567b07
Remove @Nullable from the maven plugin.
nedtwigg Mar 31, 2020
2f77a77
Revert all ned's commits since the last good commit by @source-knights
nedtwigg Apr 2, 2020
3d791da
Put the baseDir into FileLocator, and ensure that names match their "…
nedtwigg Apr 2, 2020
f5d9e07
Make the gradle tests use the same test files as the maven and lib te…
nedtwigg Mar 31, 2020
7d21bf4
Extracted `Tsfmt.locateFile` into `Tsfmt.locateLocal`.
nedtwigg Apr 2, 2020
1bb33d6
Remove maven-plugin info from the lib changelog.
nedtwigg Apr 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Expand Up @@ -10,6 +10,8 @@ This document is intended for Spotless developers.
We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`).

## [Unreleased]
### Added
* Tsfmt Maven Plugin ([#548](https://github.com/diffplug/spotless/pull/548))
### Fixed
* Javadoc for the `ext/eclipse-*` projects.
* Replace the deprecated `compile` with `implementation` for the `ext/eclipse-*` projects.
Expand Down
4 changes: 2 additions & 2 deletions README.md
Expand Up @@ -47,7 +47,7 @@ extra('java.EclipseFormatterStep') +'{{yes}} | {{yes}}
lib('kotlin.KtLintStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('markdown.FreshMarkStep') +'{{yes}} | {{no}} | {{no}} |',
lib('npm.PrettierFormatterStep') +'{{yes}} | {{no}} | {{no}} |',
lib('npm.TsFmtFormatterStep') +'{{yes}} | {{no}} | {{no}} |',
lib('npm.TsFmtFormatterStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('scala.ScalaFmtStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('sql.DBeaverSQLFormatterStep') +'{{yes}} | {{no}} | {{no}} |',
extra('wtp.EclipseWtpFormatterStep') +'{{yes}} | {{yes}} | {{no}} |',
Expand All @@ -74,7 +74,7 @@ extra('wtp.EclipseWtpFormatterStep') +'{{yes}} | {{yes}}
| [`kotlin.KtLintStep`](lib/src/main/java/com/diffplug/spotless/kotlin/KtLintStep.java) | :+1: | :+1: | :white_large_square: |
| [`markdown.FreshMarkStep`](lib/src/main/java/com/diffplug/spotless/markdown/FreshMarkStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`npm.PrettierFormatterStep`](lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`npm.TsFmtFormatterStep`](lib/src/main/java/com/diffplug/spotless/npm/TsFmtFormatterStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`npm.TsFmtFormatterStep`](lib/src/main/java/com/diffplug/spotless/npm/TsFmtFormatterStep.java) | :+1: | :+1: | :white_large_square: |
| [`scala.ScalaFmtStep`](lib/src/main/java/com/diffplug/spotless/scala/ScalaFmtStep.java) | :+1: | :+1: | :white_large_square: |
| [`sql.DBeaverSQLFormatterStep`](lib/src/main/java/com/diffplug/spotless/sql/DBeaverSQLFormatterStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`wtp.EclipseWtpFormatterStep`](lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java) | :+1: | :+1: | :white_large_square: |
Expand Down
Expand Up @@ -20,6 +20,8 @@
import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;

import com.diffplug.spotless.LineEnding;

class NodeJSWrapper extends ReflectiveObjectWrapper {

public static final String V8_RUNTIME_CLASS = "com.eclipsesource.v8.V8";
Expand All @@ -33,7 +35,7 @@ public NodeJSWrapper(ClassLoader classLoader) {
super(Reflective.withClassLoader(classLoader),
reflective -> {
final boolean firstRun = flagsSet.compareAndSet(false, true);
if (firstRun) {
if (firstRun && LineEnding.PLATFORM_NATIVE.str().equals("\r\n")) {
reflective.invokeStaticMethod(V8_RUNTIME_CLASS, "setFlags", "-color=false"); // required to run prettier on windows
}
return reflective.invokeStaticMethod(WRAPPED_CLASS, "createNodeJS");
Expand Down
2 changes: 2 additions & 0 deletions plugin-maven/CHANGES.md
Expand Up @@ -3,6 +3,8 @@
We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`).

## [Unreleased]
### Added
* Tsfmt Maven Plugin ([#548](https://github.com/diffplug/spotless/pull/548))
### Fixed
* Eclipse-WTP formatter (web tools platform, not java) handles some character encodings incorrectly on OS with non-unicode default file encoding [#545](https://github.com/diffplug/spotless/issues/545). Fixed for Eclipse-WTP formatter Eclipse version 4.13.0 (default version).

Expand Down
50 changes: 50 additions & 0 deletions plugin-maven/README.md
Expand Up @@ -193,6 +193,56 @@ By default, all files matching `src/main/cpp/**/*.<ext>` and `src/test/cpp/**/*.
```
Use the Eclipse to define the *Code Style preferences* (see [Eclipse documentation](https://www.eclipse.org/documentation/)). Within the preferences *Edit...* dialog, you can export your configuration as XML file, which can be used as a configuration `<file>`. If no `<file>` is provided, the CDT default configuration is used.

<a name="typescript"></a>

## Applying to Typescript source

```xml
<configuration>
<typescript>
<tsfmt>
<!-- optionally define which files will be formatted. -->
<includes>
<include>src/**/*.ts</include> <!-- default value if nothing is specified -->
</includes>
<!-- must specify exactly one of the following "{foo}File" or "config" elements -->
<tslintFile>${basedir}/path/to/repo/tslint.json</tslintFile>
<tsfmtFile>${basedir}/path/to/repo/tsfmt.json</tsfmtFile>
<tsconfigFile>${basedir}/path/to/repo/tsconfig.json</tsconfigFile>
<vscodeFile>${basedir}/path/to/repo/vscode.json</vscodeFile>
<config>
<indentSize>1</indentSize>
<convertTabsToSpaces>true</convertTabsToSpaces>
</config>
<!-- optionally configure following versions to use, shown values are defaults-->
<typescriptFormatterVersion>7.2.2</typescriptFormatterVersion>
<typescriptVersion>3.3.3</typescriptVersion>
<tslintVersion>5.12.1</tslintVersion>
</tsfmt>
</typescript>
</configuration>
```

Supported config file types are `tsconfigFile`, `tslintFile`, `vscodeFile` and `tsfmtFile`. They are corresponding to the respective
[tsfmt-parameters](https://github.com/vvakame/typescript-formatter/blob/7764258ad42ac65071399840d1b8701868510ca7/lib/index.ts#L27L34). See [tsfmt's default config settings](https://github.com/vvakame/typescript-formatter/blob/7764258ad42ac65071399840d1b8701868510ca7/lib/utils.ts#L11L32) for what is available.

*Please note:*
The auto-discovery of config files (up the file tree) will not work when using tsfmt within spotless,
hence you are required to provide resolvable file paths for config files.

### Prerequisite: tsfmt requires a working NodeJS version

tsfmt is based on NodeJS, so to use it, a working NodeJS installation (especially npm) is required on the host running spotless.
Spotless will try to auto-discover an npm installation. If that is not working for you, it is possible to directly configure the npm binary to use.

```xml
<configuration><typescript><tsfmt>
...
<npmExecutable>/usr/bin/npm</npmExecutable>
```

Spotless uses npm to install necessary packages locally. It runs tsfmt using [J2V8](https://github.com/eclipsesource/J2V8) internally after that.

<a name="format"></a>

## Applying to custom sources
Expand Down
Expand Up @@ -45,6 +45,7 @@
import com.diffplug.spotless.maven.java.Java;
import com.diffplug.spotless.maven.kotlin.Kotlin;
import com.diffplug.spotless.maven.scala.Scala;
import com.diffplug.spotless.maven.typescript.Typescript;

public abstract class AbstractSpotlessMojo extends AbstractMojo {

Expand Down Expand Up @@ -98,6 +99,9 @@ public abstract class AbstractSpotlessMojo extends AbstractMojo {
@Parameter
private Cpp cpp;

@Parameter
private Typescript typescript;

/** The CSS extension is discontinued. */
@Parameter
@Deprecated
Expand Down Expand Up @@ -179,7 +183,7 @@ private FileLocator getFileLocator() {
}

private List<FormatterFactory> getFormatterFactories() {
return Stream.concat(formats.stream(), Stream.of(java, scala, kotlin, cpp, css, xml))
return Stream.concat(formats.stream(), Stream.of(java, scala, kotlin, cpp, typescript, css, xml))
.filter(Objects::nonNull)
.collect(toList());
}
Expand Down
@@ -0,0 +1,105 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.maven.typescript;

import java.io.File;
import java.util.Map;

import org.apache.maven.plugins.annotations.Parameter;

import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.maven.FormatterStepConfig;
import com.diffplug.spotless.maven.FormatterStepFactory;
import com.diffplug.spotless.npm.TsConfigFileType;
import com.diffplug.spotless.npm.TsFmtFormatterStep;
import com.diffplug.spotless.npm.TypedTsFmtConfigFile;

public class Tsfmt implements FormatterStepFactory {

@Parameter
private String tslintFile;

@Parameter
private String tsconfigFile;

@Parameter
private String vscodeFile;

@Parameter
private String tsfmtFile;

@Parameter
private String typescriptFormatterVersion;

@Parameter
private String typescriptVersion;

@Parameter
private String tslintVersion;

@Parameter
private String npmExecutable;

@Parameter
private Map<String, Object> config;

@Parameter(defaultValue = "${project.build.directory}", required = true, readonly = true)
Copy link
Member

Choose a reason for hiding this comment

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

This is what I mean by "buildDir" default. It appears to not actually be readonly, and it also appears that the default does not work. One solution is for the "buildDir" to be just a regular nullable parameter, since it seems that's how you're actually using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was put in due to the tsftmt step formatter wants to have the build dir. In the Gradle impl that seems to be easier, but here in the Maven this seems to be the way. At least that Parameter(defaultValue = "${project.build.directory}" is used somewhere else (and seems to work with all the tests, except maybe the tsconfig test).

I still think the error message with tsconfig is missleading, cause when I looked at it, it was actually mentioning the found file name of the test.ts, so it does not really seem to be an error with missing inputs.

No time to look at this right now, maybe tomorrow. Thx @nedtwigg

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. My assertion is that defaultValue has no effect, because later on you say "if buildDir is null then set it to some default value". That logic, of setting a default value if it is null, is fine. My objection, perhaps mistaken, is that the parameter annotation is claiming to magically set a default value, but it isn't working. So if it isn't working, it shouldn't be there.

If there are other instances like this in the codebase, then I think they are broken too.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm skeptical that new File(".") will work well as a default value for multiproject setups, but I'm willing to merge and ship this and wait for someone to have a problem and then we can worry about fixing it.

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 think you will get null if you start this without a maven/gradle setup (e.g. in a unit test straight in IDE). Therefore I set the ".". But might be wrong

Copy link
Member

Choose a reason for hiding this comment

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

I tried deleting it, and I saw failures in the maven integration tests, which run using "real maven".

private File buildDir;

@Override
public FormatterStep newFormatterStep(FormatterStepConfig stepConfig) {

Map<String, String> devDependencies = TsFmtFormatterStep.defaultDevDependencies();
if (typescriptFormatterVersion != null) {
devDependencies.put("typescript-formatter", typescriptFormatterVersion);
}
if (typescriptVersion != null) {
devDependencies.put("typescript", typescriptVersion);
}
if (tslintVersion != null) {
devDependencies.put("tslint", tslintVersion);
}

File npm = npmExecutable != null ? stepConfig.getFileLocator().locateFile(npmExecutable) : null;

TypedTsFmtConfigFile configFile = null;

// check that there is only 1 config file or inline config
if (this.tsconfigFile != null
^ this.tsfmtFile != null
^ this.tslintFile != null
^ this.vscodeFile != null) {
if (this.tsconfigFile != null) {
configFile = new TypedTsFmtConfigFile(TsConfigFileType.TSCONFIG, stepConfig.getFileLocator().locateFile(tsconfigFile));
} else if (this.tsfmtFile != null) {
configFile = new TypedTsFmtConfigFile(TsConfigFileType.TSFMT, stepConfig.getFileLocator().locateFile(tsfmtFile));
} else if (this.tslintFile != null) {
configFile = new TypedTsFmtConfigFile(TsConfigFileType.TSLINT, stepConfig.getFileLocator().locateFile(tslintFile));
} else if (this.vscodeFile != null) {
configFile = new TypedTsFmtConfigFile(TsConfigFileType.VSCODE, stepConfig.getFileLocator().locateFile(vscodeFile));
}
} else {
if (config == null) {
throw new IllegalArgumentException("must specify exactly one configFile or config");
}
}

if (buildDir == null) {
buildDir = new File(".");
}
return TsFmtFormatterStep.create(devDependencies, stepConfig.getProvisioner(), buildDir, npm, configFile, config);
}
}
@@ -0,0 +1,48 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.maven.typescript;

import java.util.Set;

import com.diffplug.common.collect.ImmutableSet;
import com.diffplug.spotless.maven.FormatterFactory;

/**
* A {@link FormatterFactory} implementation that corresponds to {@code <typescript>...</typescript>} configuration element.
* <p>
* It defines a formatter for typescript source files.
*/
public class Typescript extends FormatterFactory {

private static final Set<String> DEFAULT_INCLUDES = ImmutableSet.of("src/**/*.ts");

private static final String LICENSE_HEADER_DELIMITER = null;

@Override
public Set<String> defaultIncludes() {
return DEFAULT_INCLUDES;
}

@Override
public String licenseHeaderDelimiter() {
return LICENSE_HEADER_DELIMITER;
}

public void addTsfmt(Tsfmt tsfmt) {
addStepFactory(tsfmt);
}

}
Expand Up @@ -39,10 +39,18 @@
import com.github.mustachejava.Mustache;
import com.github.mustachejava.MustacheFactory;

import com.diffplug.common.base.Unhandled;
import com.diffplug.common.io.Resources;
import com.diffplug.spotless.ResourceHarness;

public class MavenIntegrationTest extends ResourceHarness {
/**
* To run tests in the IDE, run `gradlew :plugin-maven:changelogPrint`, then
* put the last version it prints into `SPOTLESS_MAVEN_VERSION_IDE`. From now
* on, if you run `gradlew :plugin-maven:runMavenBuild`, then you can run tests
* in the IDE and they will run against the results of the last `runMavenBuild`
*/
private static final String SPOTLESS_MAVEN_VERSION_IDE = null;

private static final String LOCAL_MAVEN_REPOSITORY_DIR = "localMavenRepositoryDir";
private static final String SPOTLESS_MAVEN_PLUGIN_VERSION = "spotlessMavenPluginVersion";
Expand Down Expand Up @@ -118,6 +126,10 @@ protected void writePomWithCssSteps(String... steps) throws IOException {
writePom(groupWithSteps("css", steps));
}

protected void writePomWithTypescriptSteps(String... steps) throws IOException {
writePom(groupWithSteps("typescript", steps));
}

protected void writePom(String... configuration) throws IOException {
writePom(null, configuration);
}
Expand Down Expand Up @@ -173,6 +185,15 @@ private static Map<String, Object> buildPomXmlParams(String[] executions, String
}

private static String getSystemProperty(String name) {
if (SPOTLESS_MAVEN_VERSION_IDE != null) {
if (name.equals("spotlessMavenPluginVersion")) {
return SPOTLESS_MAVEN_VERSION_IDE;
} else if (name.equals("localMavenRepositoryDir")) {
return new File("build/localMavenRepository").getAbsolutePath();
} else {
throw Unhandled.stringException(name);
}
}
String value = System.getProperty(name);
if (isNullOrEmpty(value)) {
fail("System property '" + name + "' is not defined");
Expand Down