Repository navigation
Break into spotless-lib (reusable for other projects) and spotless-gradle #56
Description
Spotless now has quite a few utilities for mass string manipulation which are useful outside of a gradle plugin:
- ensuring idempotency, and working around it when there are bugs
- writing functions in an easy-to-serialize way
If we break the core functionality into spotless-lib, that would allow spotless-maven, etc. It would also speed up the spotless test suite by relying less on the excellent but slow gradle testkit. Going to explore this in the spotless-lib branch.
Activity
jbduncan commentedon Nov 29, 2016
I actually thought about this a few weeks ago, but I was unsure about the usefulness of it, so I never raised it.
I look forward to any results you find from your investigation!
nedtwigg commentedon Nov 30, 2016
I think it's an improvement. I'm gonna merge
3.xintospotless-liband start splitting it up further. I'll need a "write-lock" while I do so :) Should be done by morning PST.nedtwigg commentedon Nov 30, 2016
These are the proposed new maven coordinates for 3.0:
com.diffplug.spotless:spotless-libcom.diffplug.spotless:spotless-gradle-plugincom.diffplug.spotless:spotless-maven-pluginThe current maven coordinate is
com.diffplug.gradle.spotless:spotlessIt's tempting to not move it at all, but since spotless 3.0 is likely to already require some small changes from existing users, we might as well optimize for lack-of-confusion for future users.
My biggest design conflict is what the dependency policy for
spotless-libshould be. It would be fairly easy to brings the core deps down to zero - we need just a few parts of jgit which we could easily copy, and just a few parts of durian/guava which are replaceable. But that's where all the third-party steps will end up, so parsing logic is going to end up there... One possible solution is to havelibandlib-extra, with the only differentiator being thatlibhas no deps, andlib-extrais loosey-goosey about deps... For the sake of moving implementation along let's pretendlibis loosey-goosey about deps, but I might split it up later this week...Todos:
plugin-gradleand intolibThere's a real need to burn-down some of the docs for all this too...
nedtwigg commentedon Dec 1, 2016
com.diffplug.spotless:spotless-libnow has zero runtime dependenciescom.diffplug.spotless:spotless-lib-extrahas lots of dependencies, and more complex pieces like .gitattributes parsing and EclipseFormatter parsing.@nonnull by default in core packages.
jbduncan commentedon Dec 3, 2016
Regarding the latter TODOs in #56 (comment), I wonder if you could you clarify what they're about for me.
For example, are they about migrating non-test classes (like
LicenseHeaderStep) away fromplugin-gradletolib? Or are they about fixing them up for incremental builds?nedtwigg commentedon Dec 3, 2016
They're about moving all FormatterStep from plugin-gradle to lib, so that the they can be reused in other plugins. Hope you feel better soon :)
jbduncan commentedon Dec 4, 2016
LicenseHeaderStepis migrated as of commits 9250cf8...3ba3aeb. Its test has yet to be migrated though (as it depends onResourceHarnessin plugin-gradle).jbduncan commentedon Dec 6, 2016
ImportSorterStepandImportSorterImplhave also been migrated as of 3a92121.jbduncan commentedon Dec 6, 2016
@nedtwigg, I wonder if I may have your thoughts on how we could migrate the tests that depend on
ResourceHarnessinplugin-gradletolib.The best idea I have so far which I'd be able to implement myself, is to simply copy-and-paste the needed parts of
ResourceHarnessinto each test that depends on it, and then migrate the tests intolibafterwards. However I don't like the idea of the potential code duplication this may cause.Would it be possible to make e.g. a Gradle module in which we could chuck in all of our test utils, and have the other modules depend on it? I ask as I'm not really familiar with how multi-module Gradle builds work.
nedtwigg commentedon Dec 6, 2016
Yeah, I struggled with this a little over the weekend. There's a couple issues:
libandlib-extra, likeGoogleJavaFormatStepandEclipseFormatterStep, require an implementation ofProvisioner, and we need gradle to implement that. Which means that we really need at least some of gradle's test infrastructure in order to run thelibandlib-extratests. But not a ton of it, and just using GradleProvisioner is a lot faster than a full blown testkitgradleRunner()test.FormatterandFormatterStep. In order for the plugins to share any test infrastructure, we need some kind oftestlibwhich can depend onlib, and then itself become a test dependency for the other plugins.Here's my gut:
spotless-testlib(same naming convention used by Guava)spotless-lib, and for now also testkit, just to provide a no-hassleProvisionerimplementation.src/directory will haveResourceHarnessand other infrastructuretest/will have all the tests forlibandlib-extragradle-pluginand other plugins will have a test-only dependency onspotless-testlibI've got some progress on this, lemme know when would be a good time for me to upload without causing conflicts for you.
jbduncan commentedon Dec 6, 2016
Oh fantastic! I see that there was a lot to consider here, so many thanks for looking into it so much already.
I'm ready whenever you're ready to upload your current progress.
nedtwigg commentedon Dec 6, 2016
Roger, I'll try to put it up in a few hours.
nedtwigg commentedon Dec 7, 2016
Does 15 hours count as a few? Sorry for delay, had a lot on my plate today. Probably won't be able to write code for this for the rest of the week, but I think we've now got the final structure. I'll start migrating artifacts into jcenter to get ready for putting them into mavenCentral.
Introduced testlib in 1fd857e, and moved lots of stuff out of gradle-plugin and into lib.
I think everything is now where it belongs, except:
Should all have their implementations in
lib. A lot of tests inplugin-gradlealso belong in lib - basically everything that isn't a subclass of GradleIntegrationTest.jbduncan commentedon Dec 7, 2016
Haha, no worries! I happened to be asleep during most of those 15 hours, so it definitely counts as only "a few" in my book. :)
Cool stuff.
Okay.
Just wanted to warn you that things are getting busy for me again, so I don't know when I'll be able to tackle any of the bullet points above (or indeed the rest of the seemingly never-ending deluge of tasks we need to finish up before shipping
3.x!)So let's tackle everything one step at a time and at our own pace. :)
jbduncan commentedon Dec 11, 2016
@nedtwigg The bullet points in your last message have now been dealt with by yours truly. :)
The tests have yet to be migrated, so that's our next step.
jbduncan commentedon Dec 12, 2016
IndentStepTest has been migrated as of 2ff0364.
jbduncan commentedon Dec 29, 2016
@nedtwigg GoogleJavaFormatTest has now been... uh, "migrated", so to speak, as of commit 240fb7c.
However, I've left the contents of what was originally GoogleJavaFormatTest and is now GoogleJavaFormatIntegrationTest alone, as it's not clear to me if it should removed in any way, either fully or partially.
May I have your feedback on what should be done with
GoogleJavaFormatIntegrationTestnow, if anything at all?nedtwigg commentedon Jan 3, 2017
Looks perfect as-is. The gradle integration test is prrroobably overkill for future rules that get added when there's a nice test in lib, but it's nice to have a few end-to-end checks in the test suite. Well done!