search

ploeh / Hyprlinkr

A URI building helper library for ASP.NET Web API
MIT License
197 stars 34 forks source link

Added UrlHelper extensions and unit tests #23

Closed dlongest closed 10 years ago

dlongest commented 10 years ago

This is the correct PR to use instead of #22.

I took the gist provided by @jbogard in #21 and turned it into this PR. I put the extensions into the same namespace used by the other extensions, also added an overload that takes an IRouteDispatcher, wasn't sure if that was desirable or not. I did add some unit tests, most of which rely on RouteLinker's functionality being correct and simply comparing the results of UrlHelper.Link<> to the output of RouteLinker.GetUri. I can add more tests or otherwise make any other changes that might be necessary.

ploeh commented 10 years ago

Thank you for your contribution! It's looking good.

As a general observation, I was wondering if Link is a good method name?

According to the Framework Design Guidelines, "method names [should] be verbs or verb phrases". Although Link can also be a verb, it sounds like an imperative action (i.e. a Command), and the method in question here is a Query. Would GetLink be a better name?

dlongest commented 10 years ago

Thanks for the feedback, I hadn't thought that about Link until you mentioned it. I considered some other ideas in addition to GetLink: CreateLink, BuildLink, and ExtractLink among them, but GetLink seems like the clearest and it matches the corresponding verb in RouteLinker so I went with that.

jbogard commented 10 years ago

Link, though it doesn't match with FDG, does line up with existing Link methods on the UrlHelper class. It basically acts as an overload.

On Sunday, December 22, 2013, Daniel Longest wrote:

Thanks for the feedback, I hadn't thought that about Link until you mentioned it. I considered some other ideas in addition to GetLink: CreateLink, BuildLink, and ExtractLink among them, but GetLink seems like the clearest and it matches the corresponding verb in RouteLinker so I went with that.

— Reply to this email directly or view it on GitHubhttps://github.com/ploeh/Hyprlinkr/pull/23#issuecomment-31089037 .

ploeh commented 10 years ago

Good point about the existing Link methods on UrlHelper. If we also call Hyprlinkr's method Link, could we run the risk of a name collision?

AFAICT, not right now, but what if the product team add more overloads in the future, e.g. Link(object)...?

jbogard commented 10 years ago

Something that might be consistent with ASP.NET MVC is to use the "For" suffix: Url.LinkFor. MVC for expression-based overloads uses DisplayFor, EditorFor, etc. They don't use "For" for the UrlHelper but perhaps LinkFor would be somewhat consistent without the chance of naming collision in the future?

On Sunday, December 22, 2013, Mark Seemann wrote:

Good point about the existing Link methods on UrlHelper. If we also call Hyprlinkr's method Link, could we run the risk of a name collision?

AFAICT, not right now, but what if the product team add more overloads in the future, e.g. Link(object)...?

— Reply to this email directly or view it on GitHubhttps://github.com/ploeh/Hyprlinkr/pull/23#issuecomment-31090311 .

ploeh commented 10 years ago

It seems that there are various options for a naming convention for those extension methods, but I think that, while it may be a bit dull, GetLink may be the safest bet.

@dlongest Thank you for your contribution. I attempted to pull the code, but in Verify mode, I get this build error:

UrlHelperExtensions.cs(52,38,52,45): error CS1712: Warning as Error: Type parameter 'TResult' has no matching typeparam tag in the XML comment on 'Ploeh.Hyprlinkr.UrlHelperExtensions.GetLink<T,TResult>(System.Web.Http.Routing.UrlHelper, System.Linq.Expressions.Expression<System.Func<T,TResult>>, Ploeh.Hyprlinkr.IRouteDispatcher)' (but other type parameters do)

dlongest commented 10 years ago

@ploeh I added the comment necessary to resolve that issue, had it on one of the methods but not the other.

When I built in Verify mode afterwards, was getting code analysis errors (1 per method) to validate helper. I added guard clauses to each GetLink method and the appropriate unit tests for each. I think that takes care of all such issues.

This may not be the appropriate forum to ask this, but I believe you're using the Verify mode similarly to the BuildRelease script in the AutoFixture codebase (code analysis and so forth). As a matter of personal curiosity, was wondering why you used different approaches between the two?

ploeh commented 10 years ago

Thank you for your contribution! It's now live as Hyprlinkr 0.10.0.

When it comes to the Verify mode versus BuildRelease in AutoFixture, it is the same approach. AutoFixture projects also have Verify build modes; the BuildRelease script simply invokes MSBuild in that build mode first.

In Hyprlinkr, I hadn't gotten around to add all the command-line scripts yet, but I actually added them yesterday evening :) Thus, Hyprlinkr too has a BuildRelease.msbuild file with corresponding command line scripts.