search

spring-cloud / spring-cloud-connectors

Library to let cloud applications connect to services
Apache License 2.0
185 stars 161 forks source link

CloudException In CloudFactory.getCloud() Method #24

Closed ryanjbaxter closed 10 years ago

ryanjbaxter commented 10 years ago

Why does the method signature not have a throws declaration for the CloudException? I realize that CloudException is a runtime exception and it is not required but it seems like you would want users to always make sure they handle this exception properly.

ramnivas commented 10 years ago

@ryanjbaxter Since such a throws declaration will serve only as a documentation, I chose to add it in the javadoc section (https://github.com/spring-projects/spring-cloud/blob/master/core/src/main/java/org/springframework/cloud/CloudFactory.java#L42), where I could also express condition under which that exception is thrown.

ryanjbaxter commented 10 years ago

But if you are not reading the JavaDoc than you don't know about it. I found out about it the hardway :) I would at least add a try catch to the example in the README so people know that it is good practice to do this.

ramnivas commented 10 years ago

Updated README to clarify the point you raised (https://github.com/spring-projects/spring-cloud/commit/6c8e0435a5613fa37603bcc96ad6afbcc0a2a908).

Note that I didn't add a try/catch. While developers should be aware of the exception being thrown, I am not sure a try/catch will do much good. In the catch block, there may not be much to do but rethrow the exception or throw another exception. In either case, there is unlikely to be a graceful fallback.

ryanjbaxter commented 10 years ago

Just one small clarification you want to make. It is not only if you have not added a cloud connector it will also happen if isInMatchingCloud() returns false for all connectors. This could happen for example if you try and run the app locally.

ramnivas commented 10 years ago

I see your point. However, technically (and a few have done that for simplified testing), one could have VCAP_APPLICATION and VCAP_SERVICES env variable in the right format in a local deployment and it will all work fine. Also, one could write a connector that looks into some local information (likely, but not necessarily, some env variable). That is why I left it at "suitable for the environment".

I will address this kind of complex discussion in proper documentation that I will take on shortly. In this README, I want to cover just the common scenario.