Shorten Dep
serialization format in common cases
#3490
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3417
We do a best-effort approach where we try to reverse the
Dep.parse
logic, and at the end do a==
check to see if the round trippedDep
is identical to the original. There's some overhead with the round trip comparison - effectively for each write-read of aDep
we add an additionalread
- but this isn't a hot code path so it shouldn't be a big deal. In exchange we get a pretty good guarantee of correctness that later on when someone round-trips the sameDep
off of disk, they get the same result. We do the same forBoundDep
and re-use most of theDep
logicThe sophistication of our
def parse
anddef unparse
logic can be extended over time, but this should work well for 99% of dependencies which are simple without fancy exclusions, publication, configuration, optional, transitive, etc.Added some assertions to
ResolveDepsTests
to check the round tripping for common scenarios, both simplified and notBefore:
After:
Note that
org.scala-lang:scala-library
is still using the old verbose serialization, due to.forceVersion()
which makes it unable to be parsed/unparsed into a naked string literal as it needs a.forceVersion()
call. We could make the shorthand serialization smarter to deal with this in future, but for now this is already a huge improvement over the status quo