-
Notifications
You must be signed in to change notification settings - Fork 23
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
Implementing cached dataset querying #96
Conversation
Thanks @Shalelol I will take a look! Meanwhile could you specify which Issues this resolves? Thanks! |
Hey @masojus I've updated my initial comment to include the issues addressed so far. I also may look into addressing more of the issues related to cached datasets as my requirements grow. |
Thank you @Shalelol! And don't worry about the AppVeyor failure. I imagine it's an expected failure I need to deal with but all the tests look to have passed. I presume manual testing went well? Or are there any major corner cases to watch out for? |
@masojus So far all manual testing is working perfectly. I will inform you if any corner cases pop up. I'm also still yet to write a unit test for |
* | ||
* Credit to Chris Allen https://github.com/crallen | ||
*/ | ||
public class SnakeCaseContractResolver : DefaultContractResolver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary because the version of Json.NET we're using doesn't yet have the SnakeCaseNamingStrategy
or is it the case that even that strategy does not satisfy your needs? How does this SnakeCaseContractResolver
handle camelCase
or PascalCase
or other casing like kebab-case
, and how does that compare to Json.NET's own SnakeCaseNamingStrategy
? How does this treat names that have a JsonPropertyAttribute
explicitly specifying the name?
http://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Serialization_SnakeCaseNamingStrategy.htm
http://www.phrohdoh.com/blog/2016/09/02/newtonsoft-json-custom-property-names/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the Contract Resolver rather than Json.NET because all the attributes defined on all the existing models are using the JsonPropertyAttribute from Newtonsoft.Json. Which will only be respected when using the JsonConvert.SerializeObject()
and JsonConvert.DeserializeObject()
functions.
The SnakeCaseContractResolver
doesnt handle camelCase
, PascalCase
or kebab-case
but custom resolvers may be defined for those. The resolver is defined at serialization/deserialization time by passing it as an option to JsonConvert like so, rather than in a global setting:
JsonConvert.DeserializeObject<DatasetDefinition>(responseString, new JsonSerializerSettings { ContractResolver = new SnakeCaseContractResolver()});
This, and indeed any ContractResolver extending Newtonsoft.Json's DefaultContractResolver
will respect all JsonPropertyAttributes
Keen/Dataset/Datasets.cs
Outdated
internal class Datasets : IDataset | ||
{ | ||
private readonly IKeenHttpClient _keenHttpClient; | ||
private readonly string _cachedDatasetReleativeUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: _cachedDatasetReleativeUrl
->_cachedDatasetRelativeUrl
Keen/KeenClient.cs
Outdated
@@ -970,5 +975,39 @@ public JObject Query(string queryName, Dictionary<string, string> parms) | |||
throw ex.TryUnwrap(); | |||
} | |||
} | |||
|
|||
public async Task<JToken> QueryDatasetAsync(string datasetName, string indexBy, string timeframe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want doc comments on these public methods.
Keen/Dataset/Datasets.cs
Outdated
using Newtonsoft.Json.Linq; | ||
using Query; | ||
|
||
public interface IDataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would tend to prefer interfaces in their own file, and public API surface (types, fields, methods, etc.) should have doc comments.
Keen/Dataset/Dataset.cs
Outdated
|
||
namespace Keen.Core.Dataset | ||
{ | ||
using Query; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminds me we need to start creating some guidelines around things like using
inside and outside of namespaces and configure some stylecop rulesets: https://github.com/DotNetAnalyzers/StyleCopAnalyzers
I'll file an Issue.
Hey @masojus, I just want to apologize for the messy state of some of the code. I just realized that I didn't really communicate how WIP this Pull Request is. I had/have every intention of coming back and cleaning everything up, things such as xml comments, moving files out and scoping usings to match the rest of the project. This is still very much an in progress project at the moment as we move all of our querying to cached querying. I was just putting the WIP PR up nice and early so that it's on your radar =] Thanks for your comments so far, I will make sure to clean everything up. |
Keen/Dataset/Datasets.cs
Outdated
responseMessage.StatusCode); | ||
} | ||
|
||
return JsonConvert.DeserializeObject<DatasetDefinition>(responseString, new JsonSerializerSettings { ContractResolver = new SnakeCaseContractResolver()}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to think about this and ponder the answers to the questions in my comment above about SnakeCaseContractResolver
and what it means to always use this resolver for all Dataset definitions.
@Shalelol No worries. I'm just skimming it anyway, so I'll take a closer look at it later when you've tidied up a bit and think it's ready for prime time. 👍 Thanks! |
@Shalelol just wanted to say, thanks for working on this! We really appreciate it. |
Always using outside of namespace.
We don't need to expose this in the client.
We don't have the Travis config merged into master yet, so I wouldn't worry too much about that one. |
Because the sync ones end up calling the async ones. This way all the dataset methods on KeenClient are covered.
Coveralls likes to point out when you've been bad, but not when you're trying to improve the coverage 😞. |
I think It should hand out gold stars and/or free beers for increased coverage ⭐️ 🍻. |
Also refactored the tests to be a bit neater
Hey @masojus. I've finished implementing all the Cached Dataset issues, and I've brought the coverage of Could you please give this another review when you have time? Thanks 😃. |
Thank you! We'll get some eyes on this as soon as possible. |
Sorry for the delay here. We're going to be merging the dotnetsummer branch shortly and will address this after that so as to also port it to .NET Standard 2.0 while we're at it. |
Hello @Shalelol, we've now merged the big chunk of work we wanted to get into master before taking this PR on. Could you merge from master and update the PR? If you need help with that we can pitch in, but then we could work on getting this PR in. Thanks! |
I created branch |
See PR #128 for a continuation of this PR. |
Changes
Comments
I am moving towards strongly typed models, leveraging the convenience of the contract resolver, and trying to work with JToken as infrequently as possible, please let me know if anyone has any issues with this.
I'm trying to adhere to the naming and code styling convention as much as possible, please do let me know if I stray from that too much.
I'm quite inexperienced with unit testing, particularly with NUnit, so would love to hear any feedback or advice regarding that.
I plan to finish work on this pull request mid to late August this year (2017).
Issues addressed