Skip to content

Conversation

@pawels-optimizely
Copy link
Contributor

Summary

  • added Semantic versioning support in go on top of match registry

Copy link
Contributor

@mikecdavis mikecdavis left a comment

Choose a reason for hiding this comment

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

Organizationally it would be cleaner to have all the matchers under the same file, semver.go. Then we can have a single semver_test.go file and parameterize all of the gt, lt, ge, le, etc. All of those tests are the same test with the same setup, just slightly different assertions that can be unified.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

first pass

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm. please address before merge.

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.

A couple of quick updates.
https://github.com/optimizely/swift-sdk/pull/352/files
Can you fix the evaluator and add a few tests for beta? Thanks!

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.

LGTM. I'm going to turn on go sdk semver for FSC and see if it passes.

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit a65b933 into master Aug 21, 2020
@thomaszurkan-optimizely thomaszurkan-optimizely deleted the pawel/semver branch August 21, 2020 21:43
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.

6 participants