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

Health Checks sample 3.1 updates #16204

Merged
merged 5 commits into from
Dec 16, 2019
Merged

Health Checks sample 3.1 updates #16204

merged 5 commits into from
Dec 16, 2019

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Dec 15, 2019

Addresses #5495
Addresses #15473

Internal Review Topic

  • 3.1 sample app confirmed running.
  • Let's put the System.Text.Json approach in here but also retain the existing Newtonsoft.Json approach. A directive in the file makes it easy for the dev to switch back and forth.
  • WRT the System.Text.Json approach: I couldn't get WriteNumber to work with a directly casted object ((Decimal)item.Value). I ended up performing the cast on a separate line with (Convert.ToDecimal) to make it happy.

cc: @lol768

@guardrex guardrex requested a review from rynowak December 15, 2019 19:42
@lol768
Copy link
Contributor

lol768 commented Dec 15, 2019

LGTM! Many thanks for this, @guardrex

Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

Looks good after addressing feedback.

@guardrex guardrex merged commit c407fc4 into master Dec 16, 2019
@guardrex guardrex deleted the guardrex/samp-update-31 branch December 16, 2019 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants