GeezerVison is showing github.com — Break into spotless-lib (reusable for other projects) and spotless-gradle · Issue #56 · diffplug/spotless
Break into spotless-lib (reusable for other projects) and spotless-gradle #56DescriptionSpotless 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. Reacted by Jonathan Bluett-Duncan Activityjbduncan commented on 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 commented on Nov 30, 2016 I think it's an improvement. I'm gonna merge 3.x into spotless-lib and start splitting it up further. I'll need a "write-lock" while I do so :) Should be done by morning PST. nedtwigg commented on 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-plugin
The current maven coordinate is
com.diffplug.gradle.spotless:spotless
It'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-lib should 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 have lib and lib-extra, with the only differentiator being that lib has no deps, and lib-extra is loosey-goosey about deps... For the sake of moving implementation along let's pretend lib is loosey-goosey about deps, but I might split it up later this week...
Todos:
- Migrate tests for lib's generic stuff out of
plugin-gradle and into lib
- LicenseHeaderStep
- FreshMark
- EclipseFormatter
- ImportSorter
There's a real need to burn-down some of the docs for all this too... nedtwigg commented on Dec 1, 2016
com.diffplug.spotless:spotless-lib now has zero runtime dependenciescom.diffplug.spotless:spotless-lib-extra has lots of dependencies, and more complex pieces like .gitattributes parsing and EclipseFormatter parsing. jbduncan commented on 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 from plugin-gradle to lib? Or are they about fixing them up for incremental builds? nedtwigg commented on 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 :) Reacted by Jonathan Bluett-Duncan jbduncan commented on Dec 4, 2016 LicenseHeaderStep is migrated as of commits 9250cf8...3ba3aeb. Its test has yet to be migrated though (as it depends on ResourceHarness in plugin-gradle).
jbduncan commented on Dec 6, 2016 ImportSorterStep and ImportSorterImpl have also been migrated as of 3a92121.
jbduncan commented on Dec 6, 2016 @nedtwigg, I wonder if I may have your thoughts on how we could migrate the tests that depend on ResourceHarness in plugin-gradle to lib.
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 ResourceHarness into each test that depends on it, and then migrate the tests into lib afterwards. 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 commented on Dec 6, 2016 Yeah, I struggled with this a little over the weekend. There's a couple issues:
- A lot of FormatterStep in
lib and lib-extra, like GoogleJavaFormatStep and EclipseFormatterStep, require an implementation of Provisioner, 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 the lib and lib-extra tests. But not a ton of it, and just using GradleProvisioner is a lot faster than a full blown testkit gradleRunner() test.
- A lot of the test infrastructure we need is based on
Formatter and FormatterStep. In order for the plugins to share any test infrastructure, we need some kind of testlib which can depend on lib, and then itself become a test dependency for the other plugins.
Here's my gut:
- create a new subproject,
spotless-testlib (same naming convention used by Guava)
- It will depend on
spotless-lib, and for now also testkit, just to provide a no-hassle Provisioner implementation.
- Its
src/ directory will have ResourceHarness and other infrastructure
- Its
test/ will have all the tests for lib and lib-extra
gradle-plugin and other plugins will have a test-only dependency on spotless-testlib
I've got some progress on this, lemme know when would be a good time for me to upload without causing conflicts for you. jbduncan commented on 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 commented on Dec 6, 2016 Roger, I'll try to put it up in a few hours. nedtwigg commented on 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:
- FormatExtension.customReplace
- FormatExtension.customReplaceRegex
- FormatExtension.trimTrailingWhitespace
Should all have their implementations in lib. A lot of tests in plugin-gradle also belong in lib - basically everything that isn't a subclass of GradleIntegrationTest. Reacted by Jonathan Bluett-Duncan jbduncan commented on Dec 7, 2016
Does 15 hours count as a few? Sorry for delay, had a lot on my plate today.
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. :)
Introduced testlib in 1fd857e, and moved lots of stuff out of gradle-plugin and into lib.
Cool stuff.
Should all have their implementations in lib. A lot of tests in plugin-gradle also belong in lib - basically everything that isn't a subclass of GradleIntegrationTest.
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 commented on 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 commented on Dec 12, 2016 IndentStepTest has been migrated as of 2ff0364. jbduncan commented on 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 GoogleJavaFormatIntegrationTest now, if anything at all? added this to the This milestone has been deleted milestone on Jan 3, 2017nedtwigg commented on 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! Reacted by Jonathan Bluett-Duncan
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|