Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Improve IUrlHelper and related doc comments #4948

Closed
wants to merge 2 commits into from

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Jul 1, 2016

/// </summary>
/// <param name="actionContext">The context object for the generated URLs for an action method.</param>
/// <returns>The fully qualified or absolute URL to an action method.</returns>
/// <returns>The URL with an absolute path for an action method.</returns>
Copy link
Member

@rynowak rynowak Jul 1, 2016

Choose a reason for hiding this comment

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

I think that this is an overall improvement in the clarity. My one suggestion here would be to not repeat the part about a URL with an absolute path, and tweak the summary to be like:

Generates an absolute URL if <see cref="UrlActionContext.Protocol"/> and <see cref="UrlActionContext.Host"/> are non-<c>null</c>, otherwise generates a URL with an absolute path.

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'm struggling with the "otherwise" nature of your suggestion. URLs with absolute paths are a strict subset of absolute URLs. Will play w/ the wording a bit more ...

@rynowak
Copy link
Member

rynowak commented Jul 1, 2016

⌚ I have a few suggestions, but overall looks like a big improvement 👍

@dougbu
Copy link
Contributor Author

dougbu commented Jul 1, 2016

🆙📅 though I haven't done the "reword <summary>" part of #4948 (diff)

@dougbu
Copy link
Contributor Author

dougbu commented Jul 5, 2016

Rcv'd offline :shipit: from @rynowak

@dougbu
Copy link
Contributor Author

dougbu commented Jul 5, 2016

15f25d5

@dougbu dougbu closed this Jul 5, 2016
@dougbu dougbu deleted the dougbu/iurlhelper.docs.4245.4507 branch July 5, 2016 16:46
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