Routing and querystring formatting support

[carl-berg]

This is my proposal for issue #64

[anaisbetts]

So, I'm not super excited about the way that this has been implemented. Is there a way that you can implement this s.t. it's an optional parameter to RestService.For? I think that this feature is a pretty niche scenario, and while I want to make it work for you, I also don't want to force every user of Refit to think about this.

[carl-berg]

I could argue that with the previous solution, a user would only need to bother with it if creating a RequestBuilderImplementation directly, but it would not really be noticable otherwise.

I made a change however so that you can input an optional IRequestParameterFormatter with the call to RestService.For. Ideally i would prefer some kind of setting parameter there instead that could set other settings aswell in the future, but maybe that's just premature optimization.

[anaisbetts]

@carl-berg If we have this as a parameter to RestService.For, do we need the new Attribute then? Most web APIs will expose times all in a similar way, no?

[carl-berg]

I think it's kind of a neat feature to be able to use both. I could see formatting a single parameter would be easier using the attribute, but creating a convention that all parameters of a certain type would be formatted a certain way, then you would have to use a IRequestParameterFormatter. But if i had to pick one, the IRequestParameterFormatter way is more powerful.

[anaisbetts]

Let's add the IRequestParameterFormatter way for now and see if anyone wants to add per-attribute formatting later

[bennor]

It does seem like overkill to provide something that's capable of formatting parameters based on their type and requiring an attribute that ties it to a specific parameter. Wouldn't a Dictionary<Type, string> or similar be enough as a parameter formatter?

Then you could pass in new Dictionary<Type, string>{{typeof(DateTime), "yyyy-MM-dd"}}.

[bennor]

Ignore me - a dictionary would actually make it more complicated. Not having the attribute means you can drop the ParameterInfo from the interface though, right? Or do you think it's worth keeping since it gives you access to the name of the parameter and any other attributes it might have?

[carl-berg]

I like that you get a bit more context with the ParameterInfo but a type check probably would suffice for most cases. I could probably use already existing code to get the parameter name. Possibly from RestMethodInfo.ParameterMap

[bennor]

I don't know. I think if you're going to be passing through the name, you might as well pass the whole ParameterInfo - that way there's a bit more context if needed. It would allow someone to extend the formatting with their own attributes if they wanted to.

[carl-berg]

Any thoughts on this? I think using a settings parameter as input for RestService.For makes it easier to add customizations in the future without changing the method signature.

[bennor]

I wonder if the name IRequestParameterFormatter might be a bit confusing, since it only actually touches URL parameters. At the moment it's not touching the body of the request and I'm not sure it should -- especially for JSON, which can already be modified with JsonConvert.DefaultSettings.

[carl-berg]

You're right, the interface for RefitSettings brings little value, i removed it. I also fixed the 💥 and renamed IRequestParameterFormatter to IUrlParameterFormatter as the formatter only touches route and querystring parameters. For body parameters there's plently of configuration options in Json.NET. It could be nice however if they could be configured through the RefitSettings in the future, but that can be saved for another day.

[bennor]

Looks better to me.

Ultimately this is all up to @paulcbetts though. I'm gonna stay out of this so I'm not encouraging you to do any more work he might ask you to undo.

[carl-berg]

Hehe, I see what you mean. I think you had some valid points though, and i'm always happy to get code reviews for free 😄. Hopefully @paulcbetts agrees with our thinking too.

[anaisbetts]

Cool, I'll have a look at this soonish once I can run the tests

[anaisbetts]

This is on my list for this week

[carl-berg]

Thanks! If there's anything I can do to help out, just let me know.

[bennor]

You might be able to speed up the process by pulling the latest from upstream into your local master, rebasing your branch on top of it, fixing any merge conflicts and force pushing but if you don't know what you're doing, that could be A Very Bad Idea. ;)

[carl-berg]

I think i've done that 😸 (i'm not as fluent in git as i am in hg)

[anaisbetts]

Uninstall R#, install VsVim instead

[anaisbetts]

Merged with some fixups. Thanks @carl-berg!

[screamish]

This seems like it would scratch an itch I'm having. Any idea when this will ship? Apologies if it's bad form to ask that, don't want to sound demanding.

[anaisbetts]

@screamish No worries - I'm probably going to make a build and release it in the next few hours

[bennor]

Awesome 👍

[screamish]

🙇 - that's awesome, indeed! I hope to have something OSS using Refit soon to share.

[carl-berg]

Nice 👍