Skip to content
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

Added JsonSerializerSettings to refitSettings #80

Merged
merged 8 commits into from
Dec 11, 2014

Conversation

mburbea
Copy link
Contributor

@mburbea mburbea commented Dec 5, 2014

Refits will now use a jsonSerializerSettings object to control how to
serialize objects.

This allows the refit settings to be more finely tuned without abusing global state. That way if I want to set that my Refit for this api uses CamelCase, but for another api uses SnakeCase I don't have to worry about them colliding on each other.

Refits will now use a jsonSerializerSettings object to control how to
serialize objects.
@bennor
Copy link
Contributor

bennor commented Dec 5, 2014

Cool. I've been meaning to add this myself.

  • Can we get a test or two for this.
  • Can you update the README
  • Can you put spaces after the commas in method calls/declarations - just to keep it consistent with the rest of the codebase.
  • There are a couple of other minor style things, I'll mention inline.

}

public JsonSerializerSettings SerializerSettings { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Name this property JsonSerializerSettings, just so it's clearer it won't do anything at all for form posts.

@bennor bennor mentioned this pull request Dec 6, 2014
4 tasks
default(T);
}

public static async Task<ApiException> Create(HttpResponseMessage response)
public static async Task<ApiException> Create(HttpResponseMessage response,JsonSerializerSettings settings)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add another overload here that has the original signature? Otherwise we break API compatibility

@mburbea
Copy link
Contributor Author

mburbea commented Dec 8, 2014

I've added some text to the readme and fixed most of the spacing issues. I personally am not sure about why ApiException and RestMethodInfo are public. I think that allowing them to break isn't so bad. They seem like candidates to be private to me. Unless the intent is that people can override RestBuilderImplementation. (Which would be very arduous).

@bennor
Copy link
Contributor

bennor commented Dec 8, 2014

My guess is RestMethodInfo is public to make testing it easier. The class ApiException needs to be public so people can catch it. The Create method could maybe be internal, but I actually use it myself in a project to signify a different error condition.

might be beneficial to isolate the settings for calls to a particular API.
When creating a Refit generated live interface, you may optionally pass a
`RefitSettings` that will allow you to specify what serializer settings you
would like. This allows you to have different serializer settings for seperate
Copy link
Contributor

Choose a reason for hiding this comment

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

"separate" :trollface:

Great documentation though! 👍

mburbea and others added 3 commits December 8, 2014 18:05
@bennor
Copy link
Contributor

bennor commented Dec 9, 2014

👍 Awesome.

@mburbea
Copy link
Contributor Author

mburbea commented Dec 9, 2014

Anything else you guys see as outstanding issues?

@bennor
Copy link
Contributor

bennor commented Dec 9, 2014

A test that proves it works (and more importantly, will tell us if we accidentally break it in future) 😉

My tests make it so the default setting is configured to CamelCase
forcing the property to be used or fail.
@mburbea
Copy link
Contributor Author

mburbea commented Dec 10, 2014

I added a few unit tests based on the existing ones for the github api.

@anaisbetts
Copy link
Member

I'm into it. Thanks @mburbea!

anaisbetts added a commit that referenced this pull request Dec 11, 2014
Added JsonSerializerSettings to refitSettings
@anaisbetts anaisbetts merged commit aa39c1f into reactiveui:master Dec 11, 2014
@lock lock bot locked and limited conversation to collaborators Jun 25, 2019
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