Datasets

[masojus]

See PR #96 for details. This PR is to continue that discussion and merge with changes made in master related to all the .NET Standard work.

[coveralls]

Coverage increased (+2.1%) to 59.461% when pulling 7c3c8ef97f8657d8ea1b46c0c0d5e6d4561bf7c8 on jm_UpdateCachedDatasetsBranch into b820c2ec8315e11a344ce4f239dc2709dd042240 on master.

[masojus]

Major remaining work is captured in Issues, and we should port this all to NetStandard before release.

[coveralls]

Coverage increased (+1.9%) to 59.182% when pulling 73790051d4ccd7f99d25a79f98f9a2534bc0ca70 on jm_UpdateCachedDatasetsBranch into b820c2ec8315e11a344ce4f239dc2709dd042240 on master.

[coveralls]

Coverage increased (+4.2%) to 61.522% when pulling 74d10c3747af94ab4be22dad715e48c3950bec47 on jm_UpdateCachedDatasetsBranch into b820c2ec8315e11a344ce4f239dc2709dd042240 on master.

[masojus]

Interesting, some of the new tests failed in Travis CI. They failed in the same way on Linux and MacOS, and it seems to be a UriFormatException so it could be a form of path separator issue between Windows and *nix OSes, or perhaps a permissions issue when trying to read the canned JSON response files, but I'll look into it.

[coveralls]

Coverage increased (+4.2%) to 61.522% when pulling 81e3f4d5ddd896a04a695d3b80a5b3f124f00280 on jm_UpdateCachedDatasetsBranch into b820c2ec8315e11a344ce4f239dc2709dd042240 on master.

[Shalelol]

Hey @masojus, thanks for all the feedback, I see you've started working on majority of the problems you pointed out.

Please let me know if you need anything from me. We're currently in the middle of the final stages of a feature release this week, but I will be able to help out if there is anything outstanding after this week.

[masojus]

Hi @Shalelol. Yeah I think it's close to ready. If you have any PR feedback on this latest revision, I'd love to hear it. Otherwise I'll probably merge this in the next few days.

[Shalelol]

Hey @masojus,

Just had a quick review of your changes and it's looking really good 🔥. I'm not sure how I missed the SnakeCaseContractResolver already available in Json.NET. I would have picked it up if I had just read the comments on the snippet I used.

Also, did the datasets API always accept group_by as a string? I remember having issues sending them as strings, though it may have been something else. Looking back I wish I hadn't gone through the effort of creating those custom converters and jumping through hoops for nothing 😂.

Just one small thing I noticed, though this might be better as a new issue:

• Consider consolidating versioning across projects for Newtonsoft.Json, NUnit and Moq. Json.NET for example has both versions 9.0.1 and 10.0.3 in the solution.

[masojus]

Thanks @Shalelol!

I'll double check index_by and group_by, but I know the API Reference isn't crystal clear about what's actually accepted (something I'll get fixed as soon as I can), so I had to try it out. I'm not sure if/when it changed.

We went about trying to consolidate versions of Json.NET awhile back (https://github.com/keenlabs/keen-sdk-net/pull/44), but since then we've started transitioning to .NET Standard 2.0, and bumping versions of some of the dependencies only in some places, knowing we plan to eliminate the 3.5- and 4.5-specific projects. Once those are gone I plan to have all those dependencies at the same version 👍

[Shalelol]

@baumatron I agree with your comments regarding the design of this project. It was quite strange to for me to come into and start working, and was quite different from most of the other C# projects I have worked on, but I tried to keep my code consistent with what was already there.

Some really simple changes that could be made to drastically increase the ease of contributing to this project are:

• Moving all classes that only store data and have no logic into a separate folder (or project) called "Models" or something. At the moment they're mixed in with the code and it's very hard to find where all the actual logic is.
• Offloading all the work around building the URL and Query Parameters as well as checking the HttpStatus code to the KeenHttpClient and then extending the KeenHttpClient into a QueriesHttpClient, DatasetHttpClient, EventCollectionHttpClient, etc, so things like building urls for the API endpoints are not tangled up with other business logic.

Doing this would result in having a series of classes sitting next to each other (Queries, Datasets, Event, EventCache, EventCollection) whose roles are very clear, and whose code is very straight to the point, with all non-business logic nonsense abstracted away (preferably in some generic way, as you said) via the HttpClients, and potentially some helpers.

[masojus]

@Shalelol Agreed. @baumatron and I also inherited the bulk of that code, and we created Issues #55 and #56 to basically cover exactly those points, so we understand your sentiment and are glad to hear our thoughts validated 😄

[baumatron]

@Shalelol All great ideas! I also have been trying to keep with the existing design of the SDK until there's an opportunity to do a larger refactor as you describe. Would love to see that in the future.

[coveralls]

Coverage increased (+2.7%) to 72.708% when pulling f73aa1ddc010a0cd80b7545f7b50550cd76a9876 on jm_UpdateCachedDatasetsBranch into 676e52fcd966db578103f74fa51b525c757df25e on master.