-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: use Semver compatibility for dependency info #101
Conversation
...-module/src/main/java/org/terasology/gestalt/module/dependencyresolution/DependencyInfo.java
Show resolved
Hide resolved
gestalt-module/src/main/java/org/terasology/gestalt/naming/SemverExpression.java
Show resolved
Hide resolved
|
||
public SemverExpression(String source) { | ||
this.source = source; | ||
this.expression = ExpressionParser.newInstance().parse(source); |
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.
TODO: error handling
or maybe using a documented exception here, and error handling in DependencyInfo?
@@ -125,7 +112,7 @@ private void populateConstraints() { | |||
Module versionedModule = registry.getModule(name, version.getVersion().get()); | |||
DependencyInfo info = versionedModule.getMetadata().getDependencyInfo(dependency); | |||
if (info != null) { | |||
constraintTable.put(version.getVersion().get(), new CompatibleVersions(info.versionRange(), info.isOptional() && !optionalStrategy.isRequired())); | |||
constraintTable.put(version.getVersion().get(), new CompatibleVersions(info.versionPredicate(), info.isOptional() && !optionalStrategy.isRequired())); |
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.
I'd love to have a uniform code formatter...
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.
what prompted that thought on this line in particular?
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.
hm, probably the length of that line 🤔
I leave the two open questions for later. I do believe that following any common way to define versions would be a reasonable choice (be it similar to npm's advanced range syntax or resembling gradle's version declarations). |
Version 7 introduced the use of the Semver library (#80) for some functions of version handling. It changed the behavior of
getNextVersion
methods (as mentioned in its PR), and that left DependencyInfo returning an unsatisfiable version range when a module'sminVersion
targets a snapshot. (#100)gestalt/gestalt-module/src/main/java/org/terasology/gestalt/module/dependencyresolution/DependencyInfo.java
Lines 84 to 89 in a51ba07
Rather than try to re-implement the semantics of satisfying semantic versioning in the DependencyInfo and VersionRange classes, I propose using more of the Semver library's methods for this.
This PR is a step in that direction. It keeps the VersionRange class intact to preserve compatibility, but if a DependencyInfo was defined with only a minimal version, instead of building a gestalt.VersionRange for it, we compile a Semver expression that will determine what matches.
Dependencies: #98. If that is not merged yet, you can review using this diff: v7/test/java11...v7/feat/semverComparison
Questions for Review
minVersion
, should we allow module metadata to define its own expression?