Closed Shalelol closed 6 years ago
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 GetDatasetDefinition
but will be doing so shortly.
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.
@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.
We don't have the Travis config merged into master yet, so I wouldn't worry too much about that one.
Just updated the opening comment with issues. I plan to implement #88, #91 and #92 for completeness.
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 ⭐️ 🍻.
Hey @masojus. I've finished implementing all the Cached Dataset issues, and I've brought the coverage of Keen.Core.Dataset
up to 91%. I've also gone through and added XML comments to everything that is public facing.
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 jm_UpdateCachedDatasetsBranch
to get this up to date with master
and continue the conversation. I'll probably close this PR and we can finish polishing that off in the new PR.
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
92 - Delete Cached Dataset
91 - List Cached Dataset Definitions
90 - Retrieve Cached Dataset Results
89 - Get Cached Dataset Definition
88 - Create Cached Dataset