-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Remove the dependencies.lock from git and make them re-generated when building project #1180
Conversation
… building project.
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.
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.
This looks good to me as well. I don't think the dependency.lock adds a lot of value. If there are any issues, then you would need to go deeper in the dependency tree anyway :)
build.gradle
Outdated
@@ -103,6 +103,7 @@ subprojects { | |||
compileJava { | |||
options.encoding = "UTF-8" | |||
} | |||
compileJava.dependsOn(generateLock, saveLock) |
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.
We should remove the locking plugin as well, right?
I agree with doing this. Locking is really helpful when building a final app so you can ensure tests run with the same classpath that will be used in production. For a library like this, it makes far less sense. Huge lock files and confusing behavior when updating versions is not worth it. Before merging, I think we should remove the dependency lock plugin as well. |
+1 |
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.
+1 from me too
Looks like there isn't much more discussion. I'm going to merge this. Thanks @openinx and to everyone that took the time to review! |
This patch will address the issue from #1176.