org-arl / fjage

Framework for Java and Groovy Agents
https://fjage.readthedocs.io/en/latest/
Other
26 stars 13 forks source link

Add option to reject various APIs and added new agents API #298

Closed notthetup closed 8 months ago

notthetup commented 8 months ago

With this feature, parameter set/get can be done using the construct

t = await device.get("time").catch(() => {console.warn("couldn't get time")})
notthetup commented 8 months ago

Love this! Ideally we'd do the same also in other gateways, I suspect this would lead to much more robust code and simplify debugging.

The major improvement here is the disambiguation between a failed agent.parameter request and one where the parameter value is actually null. Similarly for agentForService and other operations.

It would be great to extend this to other gateways, but it's slightly easier with JavaScript since it doesn't have blocking operations, so we have to wrap them in a Promise.

In other languages we'll have to find the proper native (hopefully) mechanism to do this over blocking calls and not make it too verbose and awkward to use.

Maybe backwards compatibility is better addressed using semantic versioning? Supporting both options in the same code is a maintenance issue, and changing the default is going to be breaking anyway.

Yes. Agreed. I thought about this. But one awkward issue here is that we bundle fjage.js into fjage.jar as well, so that would require us semver upgrade fjage.jar as well, since we don't want a user to upgrade fjage.jar and automatically get a new fjage.js which might break things with their app. I haven't found a nice way to pin dependency between fjage.js and fjage.jar.

I find this name somewhat confusing, probably because my first thought upon reading this was that "Error" would refer to the "error" in "throw error instead of returning null". Maybe returnNullOnFailedResponse?

returnNullOnFailedResponse sounds good. I'll update.

ettersi commented 8 months ago

Maybe backwards compatibility is better addressed using semantic versioning? Supporting both options in the same code is a maintenance issue, and changing the default is going to be breaking anyway.

Yes. Agreed. I thought about this. But one awkward issue here is that we bundle fjage.js into fjage.jar as well, so that would require us semver upgrade fjage.jar as well, since we don't want a user to upgrade fjage.jar and automatically get a new fjage.js which might break things with their app. I haven't found a nice way to pin dependency between fjage.js and fjage.jar.

If fjage.js is part of fjage.jar, then it shouldn't be surprising that a breaking update for fjage.jar also breaks fjage.js, no? I do see the issue that this would force us to update all of unet-web at once, though.

notthetup commented 8 months ago

I do see the issue that this would force us to update all of unet-web at once, though.

Also, we won't be able to pick up any new functionality in fjåge (like https://github.com/org-arl/fjage/pull/300 or https://github.com/org-arl/fjage/pull/301) until the entire unet-web updated to use the new API which might be a while.

Hence, I thought of using the "deprecated" approach and hopefully removing the original functionality it in a future release when we have the chance to up-rev fjage.jar as well.

ettersi commented 8 months ago

Hence, I thought of using the "deprecated" approach and hopefully removing the original functionality it in a future release when we have the chance to up-rev fjage.jar as well.

Sounds reasonable 👍