Versioning contract module #8
Conversation
|
Looks promising! 👍 Now we just need to determine all old and new API elements, call it, and see if it works... 😉 |
I was already thinking about it. I imagine that the best orchestration for that would be as part of CI pipeline. I'd start with Maven and Gradle plugins, that would provide 2 or 3 tasks or targets. First task would be Second would be Third would be Given paths would be defaults, tasks would have to be configurable to support inclusion and exclusions of source sets and custom descriptor paths and names. Buildscript writer would have to decide how to orchestrate those two steps - manually, on developer machine, on CI server, etc, but it would provide the most basic steps needed. |
apiguardian-contract/build.gradle
Outdated
| targetCompatibility = 1.8 | ||
| } | ||
|
|
||
| //todo: can we use Lombok? it will clean up implementation a bit, but I'm not sure whether we want any dependencies |
There was a problem hiding this comment.
Would we be using Lombok to generate immutable data holder classes? If so, and if the API Guardian team would be willing to adopt it, then AutoValue would arguably be a better choice, since it doesn't hack into javac unlike Lombok. (Lombok is known to have interoperability problems with other tools like error-prone, so IMO it's not the safest tool to use.)
| boolean majorVersionChanged, boolean minorVersionChanged){ | ||
| return previousStatePredicate.test(previousState) && nextStatePredicate.test(nextState) && | ||
| (!requiresMajorVersionIncrement || majorVersionChanged) && | ||
| (!requiresMinorVersionIncrement || minorVersionChanged); |
There was a problem hiding this comment.
A bug: should be: doesnt require minor change or minor change happened or major change happened
| public static List<StateTransitionRule> rules = asList( | ||
| onMajorVersionIncrement(ALL_STATES, ALL_STATES), | ||
| anytime(asList(INTERNAL, EXPERIMENTAL), NONE), | ||
| anytime(DEPRECATED, INTERNAL), |
| STABLE; | ||
|
|
||
| public boolean featureExists(){ | ||
| switch (this){ |
There was a problem hiding this comment.
What about this?
return this != NONEThere was a problem hiding this comment.
Yes, it does the job, but I wanted to keep consistency between descriptive methods. I'd say that compiler should optimize it to equivalent code in both cases and the performance trade-off isn't worth inconsistent code, especially that this tooling isn't supposed to be super-fast, but rather super-clear.
|
@apiguardian - I could use some style hints here. Please see TODOs, mainly about VersionComponentChange and APIElementState. @anyone - does anyone have an idea for tests that won't be basically re-write of the whole state graph? |
THIS SHOULDN'T BE MERGED YET!
This needs some aesthetical work yet, but I'm creating a pull request to be able to discuss the ideas and solutions.
See issue #7 .