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

Adding try behavior for loading json reader #1930

Merged
merged 5 commits into from Sep 28, 2018

Conversation

Projects
None yet
4 participants
@maiqbal11
Contributor

maiqbal11 commented Sep 25, 2018

Adding try behavior when loading JObject to improve user experience with respect to #1702.

@dnfclas

This comment has been minimized.

dnfclas commented Sep 25, 2018

CLA assistant check
All CLA requirements met.

@maiqbal11 maiqbal11 requested review from paulbatum and mathewc Sep 25, 2018

@paulbatum

This comment has been minimized.

Member

paulbatum commented Sep 25, 2018

As discussed, I suspect this is going to have the same behavior as the previous implementation in that an exception is still thrown and caught. I'll wait for an updated PR.

@paulbatum

Stupid JSON.NET not having a TryParse... oh well, LGTM!

@@ -59,6 +59,11 @@ public static void SetOwner(Guid functionOwner, JObject token)
return null;
}
if (json == null)

This comment has been minimized.

@mathewc

mathewc Sep 27, 2018

Contributor

nit: just move this check into the if clause below

@@ -85,6 +85,17 @@ internal static JsonTextWriter CreateJsonTextWriter(TextWriter textWriter)
return jsonWriter;
}
public static bool IsValidJObjectInput(string input)

This comment has been minimized.

@mathewc

mathewc Sep 27, 2018

Contributor

nit: recommend renaming this to IsJsonObject - we're not really checking whether it's valid json, so "valid" is a little strong

@mathewc

Looks good!

@maiqbal11 maiqbal11 merged commit a1ecc11 into Azure:dev Sep 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment