Skip to content

feature: add semantic version types and test #386

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

Merged
merged 19 commits into from
Aug 19, 2020

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Jul 15, 2020

Summary

  • add greater than or equal and less than or equal for numbers
  • add semantic version match types.
  • Semantic versioning rules given by https://semver.org/ are followed.

Test plan

Added unit tests

@coveralls
Copy link

@coveralls
Copy link

coveralls commented Jul 15, 2020

Pull Request Test Coverage Report for Build 1485

  • 122 of 139 (87.77%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 89.369%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core-api/src/main/java/com/optimizely/ab/config/audience/match/GEMatch.java 7 9 77.78%
core-api/src/main/java/com/optimizely/ab/config/audience/match/LEMatch.java 7 9 77.78%
core-api/src/main/java/com/optimizely/ab/config/audience/match/SemanticVersionGEMatch.java 9 11 81.82%
core-api/src/main/java/com/optimizely/ab/config/audience/match/SemanticVersionGTMatch.java 9 11 81.82%
core-api/src/main/java/com/optimizely/ab/config/audience/match/SemanticVersionLEMatch.java 9 11 81.82%
core-api/src/main/java/com/optimizely/ab/config/audience/match/SemanticVersionLTMatch.java 9 11 81.82%
core-api/src/main/java/com/optimizely/ab/config/audience/match/SemanticVersion.java 43 48 89.58%
Totals Coverage Status
Change from base Build 1473: -0.05%
Covered Lines: 4077
Relevant Lines: 4562

💛 - Coveralls

@msohailhussain msohailhussain marked this pull request as draft July 16, 2020 20:03
@mnoman09 mnoman09 marked this pull request as ready for review July 29, 2020 13:02
@msohailhussain msohailhussain marked this pull request as draft August 6, 2020 21:22
@mnoman09 mnoman09 marked this pull request as ready for review August 10, 2020 15:51
Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit

return 0;
}

public static class SemanticVersionExtension {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good. One nit. Why the inner static methods? They could be part of the semantic version and collapse some logic. so, isBuild() {self.version.contains... would be against semantic version, isPreRelease, and split would just use self.version. Perhaps there is something I'm missing?

Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a little nit:
Sorry to be a pain, but, I also meant that you would not need to pass in a string variable in isPreRelease, isBuild, or splitSemanticVersion. You would just split on the semantic version itself (implied self.version). Does that make sense? All you have to do is remove the parameter so it uses the property. Thanks!

return 0;
}

String[] targetedVersionParts = splitSemanticVersion(targetedVersion.version);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would change to targetVersion.splitSemanticVersion() and self.splitSemanticVersion().

}

private boolean isPreRelease(String semanticVersion) {
return semanticVersion.contains(PRE_RELEASE_SEPERATOR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove string in isPreRelease and use as semver.isPreRelease. the scope can be public or protected. thanks!

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit 312a1d5 into master Aug 19, 2020
@thomaszurkan-optimizely thomaszurkan-optimizely deleted the mnoman/semVersion branch August 19, 2020 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants