Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@shana
Copy link
Contributor

@shana shana commented Oct 8, 2015

Fixes #121

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, url is effectively null, right given the definition of IsValidUri.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's no real change between url != null and IsValidUrl, but it's more explicit what it's checking for (if it's a valid url that was successfully parsed from a string value).
And you never know, maybe the definition of what constitutes a valid url might change in the future ¯_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(i.e., it might be a non-null uri that's invalid for other reasons)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my point is does this change simply push the nullref exception further up the stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no, all the callers are prepared to get a null back from this method, null is a valid return.

haacked added a commit that referenced this pull request Oct 8, 2015
…ever-throw

ToRepositoryUrl should never throw, it's a conversion helper
@haacked haacked merged commit 057e8cf into release-1.0.15 Oct 8, 2015
@haacked haacked deleted the shana/121-url-conversion-should-never-throw branch October 8, 2015 18:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants