-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add setting versionPolicyModuleVersionExtractor #140
Add setting versionPolicyModuleVersionExtractor #140
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.
Thank you @jgogstad for opening the PR!
The changes look good to me. I have one comment about the choice of using a total function vs a partial function.
Also, could you please add documentation (in the README) about the new key, and its specification?
sbt-version-policy/src/main/scala/sbtversionpolicy/SbtVersionPolicyKeys.scala
Outdated
Show resolved
Hide resolved
sbt-version-policy/src/main/scala/sbtversionpolicy/SbtVersionPolicySettings.scala
Show resolved
Hide resolved
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.
Thank you @jgogstad for addressing my comments. After thinking more about it I found some issues with the current implementation, please read my new comments below.
sbt-version-policy/src/main/scala/sbtversionpolicy/SbtVersionPolicySettings.scala
Show resolved
Hide resolved
...version-policy/src/sbt-test/sbt-version-policy/dependency-source-incompatibilities/build.sbt
Outdated
Show resolved
Hide resolved
I was bit unsure where you want the default value for Thanks for the thorough review and sorry about forgetting to update the test case |
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.
Thank you @jgogstad, this looks great to me!
Just a couple of things before we merge the PR. Could you please:
|
… version number schemes This setting allow users to control how version numbers are parsed before they are checked for compatibility
37b5e6f
to
16000ab
Compare
Thanks, done. I kept the original description for the |
Thank you! |
Add setting that lets users inject parsing logic for module revision to version numbers. This will work as an escape hatch for situations where dependencies have non-standard version schemes. See linked issue for discussion.
This PR is reviewable on its own, so not making any attempts to fix the dynver custom separator issue now. Reviewers should have in mind whether this new setting could be a good fit for that issue too though. I don't think it makes sense to consider this setting for
versionPolicyPreviousArtifacts
orversionPolicyIgnored
, but please give feedback on this as well.fixes #139