-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fixes #613. #614
Fixes #613. #614
Conversation
@Particular/serviceinsight-maintainers Please review. |
@HEskandari why was this tagged as being blocked? |
@@ -15,6 +15,11 @@ public void ConnectTo(string url) | |||
{ | |||
Url = url; | |||
|
|||
if (url == null) |
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.
Would it be better to throw an exception in this case? I guess we're fixing the problem at the call site below?
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.
Don't think so. This is called on application start as well which means if an exception is thrown the app will crash.
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.
The only usages I see are
connection.ConnectTo(serviceUrl); Uri.IsWellFormedUriString
returnsfalse
when passed anull
string.connectionProvider.ConnectTo(configuredConnection); GetConfiguredAddress
cannot return null.connectionProvider.ConnectTo(existingConnection);
The code smell I'm detecting is the setting of the Url
property to null
and then avoiding the clearing of controls, refreshing etc. when that is the case. I guess the overall question is: does the behaviour of this method make sense when the passed in URL is null, in the context of the expectation of all consumers of this type?
Doing a null check on the url SI connects to. Also doing that on OnInitialize to fix the auto-connect on startup issue.
Fixes #613