-
Notifications
You must be signed in to change notification settings - Fork 3
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 ability to subscribe to service events & associated artifacts & tests #31
Conversation
@@ -20,7 +20,7 @@ apply plugin: 'com.github.johnrengelman.shadow' | |||
// | |||
group = 'io.vantiq' | |||
archivesBaseName = 'vantiq-sdk' | |||
version = '1.0.32' |
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 don't seem to have been using the "minor" version here to indicate an API addition, so I continued that tradition. Let me know if that's not what we want. That is, since this adds semantic capability (although it doesn't add any new methods -- it changes the allowable values & associated behavior), should we move the version from 1.0.32 to 1.1.0/1.1.1? It's an upward compatible change, but users using this version shouldn't downgrade if they use this capability.
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 would like to establish a new tradition -- when we add new features (without breaking the existing API), we bump the feature (aka minor) version and not the patch version. So yes, IMO this should be 1.1.0
.
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.
Tradition established...
We use a bit of sleight of hand to have the service event publication actually cause the same underlying topic event to get published. This greatly simplifies test setup.
embedded.addProperty("b", 2); | ||
message.add("o", embedded); | ||
|
||
// Publish to service event. |
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.
Here, we "cheat" a bit. The following explains what's actually going on as doing things this way simplifies the test setup. Don't need some app, etc., to respond. We just use the plumbing.
Added to semantic API. Ergo, change minor version.
docs/api.md
Outdated
@@ -909,8 +909,8 @@ void vantiq.subscribe(String resource, String name, TypeOperation operation, Sub | |||
|
|||
Name | Type | Required | Description | |||
:--: | :--: | :------:| ----------- | |||
resource | String | Yes | The resource event to subscribe. Must be either SystemResources.TOPICS.value(), SystemResources.SOURCES.value() or SystemResources.TYPES.value(). | |||
name | String | Yes | The resource name that identifies the specific resource event. For topics, this is the topic name (e.g. '/my/topic/'). For sources, this is the source name. For types, this is the data type name. | |||
resource | String | Yes | The resource event to subscribe. Must be either SystemResources.TOPICS.value(), SystemResources.SOURCES.value(), SystemResources.SERVICES.values(), or SystemResources.TYPES.value(). |
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.
Should that be SystemResources.SERVICES.value() instead of SystemResources.SERVICES.values()? It looks inconsistent with the others.
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.
Should that be SystemResources.SERVICES.value() instead of SystemResources.SERVICES.values()? It looks inconsistent with the others.
Yes, thanks. A should've cut-n-paste'd error!
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.
LGTM (except for one possible little typo)
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.
LGTM (modulo Steve's suggestion)
Fixes #30
Adds service event subscriptions. Test (unit & integration) updated to test this capability.
Fixes #29
Added mocked & integration tests for
publish(SERVICES...)
to test publishing to service event.