-
Notifications
You must be signed in to change notification settings - Fork 459
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
Adding npm-based formatters typescript-formatter (tsfmt) and prettier #283
Adding npm-based formatters typescript-formatter (tsfmt) and prettier #283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure of this is great! You're definitely heading in the right direction.
- I just pushed up Formatter is now AutoCloseable, so that facilitates
FormatterFunc.Closeable
works. #284 which makesFormatterFunc.Closeable
actually work. - I would probably change package
nodebased
to justnpm
. I think that's the brand that user's will know best. - I'd probably move your subpackages into the root package as well. I think their names are distinct enough to make it clear.
- This will allow us to make more of the implementation package-private. The less API we have to maintain the better.
All of the above is nitpicks though, feel free to leave as-is. My only substantive feedback is about the PrettierOptions
and TsOptions
classes. They look laborious and error-prone to create and maintain, and in the end they just assign values to a map. How about if the state just has TreeMap<String, Object>
, and when you create the FormatterFunc you just iterate over the entries? That way users can use all the groovy / kotlin map literals they're used to, and we can delegate to tsfmt and prettier for the documentation.
.map(File::getName) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
URL[] jarUrls() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added method has no usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's true for now. I had added the method to try using a custom-compiled j2v8 version. The now used v.4.6 is 2 years old and incorporates a rather old npm version. However, if we would go down the road with a custom built j2v8 version, we would still need to discuss on how to have those os-specific jar files available on client-machines... Any thoughts on this part? Are we fine with a 2 year old j2v8 version or do we need to something about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with a 2-year old j2v8 (Java 8 is already 4+ years old). If you want to publish your own j2v8 binaries under the spotless namespace, I'd be willing to help you, but I think it'd be better to help j2v8 itself get them published if a 2-year old j2v8 can't get it done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the 2-year-old version is working with prettier and typescript-formatter, so there's no need to act right now. If we intend to upgrade those versions some day or even let the user decide which version to use, we might night to get back to this topic. For now, I'll remove the change.
import java.util.Objects; | ||
import java.util.StringJoiner; | ||
|
||
public class Reflective { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this package-private if we can. The less API surface-area we have to maintain the better. This looks useful, but ideally we can accomplish this PR with the only new API being the steps, and keep as much of the implementation private as we can. Might be nice to move this utility into lib
as API and improve our other reflection code with it someday...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, regarding moving this to lib
! However, if we go down that route, we should seriously consider mimicking the API of JOOR, which AFAICT has a more user-friendly API than the current one that Reflective
exposes.
Heck, we might even consider shading JOOR into lib
instead. However, I personally have no experience with incorporating shaded libraries into a Java project, so the only advice I could give would be to look into a Gradle plugin like https://github.com/johnrengelman/shadow and see where that leads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, my comment above is out-of-scope for this PR and something to consider only when we're ready to move this class in the future. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just raised a new issue for this: #301
Optional.ofNullable(getRequirePragma()).ifPresent(val -> v8Object.add("requirePragma", val)); | ||
Optional.ofNullable(getInsertPragma()).ifPresent(val -> v8Object.add("insertPragma", val)); | ||
Optional.ofNullable(getProseWrap()).ifPresent(val -> v8Object.add("proseWrap", val)); | ||
return v8Object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now that all the fields of this object are boxed instead of primitive so that you can use nullability to optionally set fields. We should comment the file to indicate that:
All fields are boxed (e.g. Integer instead of int) to allow null values. A null value indicates that the default value will be used.
#284 has been merged, so I merged master into your branch so that the |
I see the advantage and elegance of that approach, however, it completely exposes the implementation internals to the user. What are your opinions on that aspect? |
I read that as "we have full responsibility for". Spotless integrates with a ton of different formatters. The only way that we're able to keep up is by delegating raw access to the formatters as much as possible. Our main value-add is playing "switchboard", and the less code we write to accomplish that goal the better.
Giving a user power inevitably means giving them the opportunity to shoot themselves in the foot. But in this case, it looks like the number of footguns is relatively small. One way to make this just as safe, but with less responsibility is something like this: for (String forbiddenOption : Arrays.asList("dryRun", "replace", "verify")) {
if (options.contains(forbiddenOption)) {
throw new IllegalArgumentException("Spotless TsFmt does not work if you set '" + forbiddenOption "');
}
} |
I like the black-list approach for the options. Will try to implement this today/tomorrow. 👍 I have so far applied some of the proposed changes: renaming and moving to Right now I have issues running the test-suite locally, seems like the TestProvisioner is using a cache folder, that is actually empty. Is there a task I can run to cleanup/re-init the cache used by the provisioner? Or a folder I can delete myself? |
I think deleting
|
that helped, thanks for pointing it out! |
as discussed in PR diffplug#283
This PR is looking great to me. I think the next thing to work on is probably the docs in Once all of the |
53e5ba4
to
07a3b5c
Compare
@nedtwigg I think I now have successfully removed all The Step-Implementation is now pretty far, at least from my perspective. Next up would be integrating the steps into gradle-/maven-plugin and - as you pointed out - updating the documentation. |
I'm currently working on the Gradle-Integration. For |
Explicit control is definitely the easiest to build. Regardless of whether some users want an implicit option, they could definitely make do with explicit control at first. If we build explicit control first, it doesn't stop us from adding implicit detection later. So I would say that our target for v1 should be the easiest thing, which luckily also happens to be the best thing for some users anyway. |
This is getting close! I added a (failing) integration test and docs. Here's how I would go about it:
At present, it seems that the step is not as simple as it thinks it is. I had to fix some NPE's to get the extension test to run at all, and it still seems to not respond to input parameters as users would expect. Very close though! |
Thanks for your help on this. I'm actually in the middle of writing the docs and (while doing that) discovering the NPEs that you probably hit in the integration test. will look into it! |
Roger, if I caused conflicts feel free to force-push over them - my intent is just to show examples :) |
3d8e3e0
to
7b96736
Compare
I have now commited a first draft for the documentation (and have adapted the implementation so that it fits the documentation 😉) further, I've taken your integration test and adapted/extended it to check tsfmt and prettier. I currently don't understand why the ci build is failing - I hoped to fix it by merging the current master, but that did not help... any ideas? |
(and please feel free to provide feedback on the documentation part or directly correct my english mistakes 🙈) |
prettier.io tsfmt
- a test dependency in testlib on lib-extra causes a cyclic dependency failure in eclipse - the only reason why lib's tests are in testlib is to fix a similar cyclic concern: lib tests -> testlib testlib -> lib so our fix was to just put lib's tests into testlib's tests. That workaround is not necessary for lib-extra.
+ spotlessApply
6ee4e77
to
a662f48
Compare
a662f48
to
ade884a
Compare
Thanks for the reminder, has been adapted.
Sure thing! Running on windows seems ok so far, however, I cannot get the ingration test for prettier to run on windows. Receiving the error:
(looks a lot like similar issues found on the web) -> it must have something to do with piping stdin/out to/from gradle(runner) but I cannot really grasp what the problem is. Maybe someone else has any idea? |
Didn't mean to approve, was still checking :/ |
Worth mentioning in the README that the new formats make use of j2v8? I didn't expect that Spotless (indirectly) would create files in my home dir (e.g. |
I wasn't able to review much of your code in the end due to limited time on my part, so sorry about that! But from what I can tell, structurally things look rather good, and although I did find three nits (see comments below), they're not important and can be left if you feel burned-out. Overall, I think this is a very, very good PR, especially for delivering such a complex and potentially very useful feature, so well done! 👏 |
Yeah, maybe we can add that to the sections where we remark that nodejs is required? |
Sounds good. |
unless travis picks something up, the PR seems pretty ready from my end. Thanks to all reviewers! |
4dc8831
to
c341a9e
Compare
c341a9e
to
807bf7b
Compare
LGTM, thank you! |
…-in-j2v8 # Conflicts: # plugin-gradle/README.md # plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java
Sorry for the delay, I picked up a flu this weekend which has set off a cascade of schedule failures for me. I'll get to this ASAP. |
Sorry for long delay, this has been shipped in |
Fantastic! Well done for all your work on this @simschla. 🎉🎊 |
Can anyone tell me if it can be changed to not create the libj2v8 file in the home folder but somewhere else? This is disturbing our teams and I fear they might dismiss the whole tool if we cannot fix this. Also @nedtwigg I think this old discussion above about J2V8 between you and @simschla is related to what I said in #555 This limits us to prettier.io version <= v1.19.0 Thanks and kind regards |
Actually it is a bit hard to change the libj2v8 location, because the configuration in that old version of J2V8 is different from what it is doing these days (and no release tag/branch in there), also Spotless does not use a way to call J2V8 with setting that directory but use the build in defaults, so I will add my findings here to make it easier for others: So the workaround is:
|
@source-knights see #586 for a proposed solution |
The goal of this PR is to add two new formatter steps to spotless
This enables spotless to format typescript and all the languages supported by prettier.
The state of the code is currently in draft. A lot needs to be done, but I start the PR now, to be able to discuss on the code rather than in issue #119