-
Notifications
You must be signed in to change notification settings - Fork 46
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
Extract dependency free API and tests #71
Conversation
@asbjornu would appreciate initial feedback on direction here. Basically the approach has been to introduce a new At the moment, only Conformance tests are implemented - there are others which need to be completed, but I wanted to stop at this point and see if you're comfortable with the overall direction. If so, next steps would be:
|
Thanks for taking on this, @goofballLogic!
I'm not quite sure about the architectural direction taken here. If I were to tackle this myself, I would probably introduce an intermediary object model in the core namespace that knew nothing about JSON or parsing, that could then be built as a result from a parser using different engines in projects such as I thus feel that having a
I think this TODO-list is reasonable. 👍 |
Just to clarify am I correctly understanding:
|
To help elucidate the above question @asbjornu , the intent was to essentially rewrite by refactoring (which I think you preferred on the whole). The idea of extracting a .Raw namespace was to establish a dependency free API (string-based, for internal use) which would adhere very closely to the existing Newtonsoft API.
Step 1: Initially this string-based API is dependent on the Newtonsoft implementation (along with the Newtonsoft-based API),
Step 2: but the whole point of establishing a string-based API was to then create a string-based implementation, and then finally to make the Newtonsoft-based API dependent on the string-based API and implementation.
Step 3: In this way we can finally extract the Newtonsoft-based API into a separate (optional) package depending the now dependency-free one.
Once this is accomplished, we should then be free to refactor the internal implementation to be pluggable for other engines (as you mentioned). One way to do that could be by creating an entirely abstract skeleton to be the core of the product. The problem with this refactoring approach is the necessary creation of the .Raw namespace as I do not see any other way to introduce a dependency-free API into the library. Do you have a good alternative? |
What I'm thinking is that objects such as |
Yes that is very much the intention here. First step is to provide a non-JToken user API, then extract JToken from the logic itself |
Ok. So this intermediary object model is going to be |
My thought was that the interface (API) of the library would be string based. Internally we could use a proper object model (e.g. Dictionaries produced by https://github.com/zanders3/json). Have changed the text diagrams above to say "POCO-based implementation" where before it read "string-based implementation" In "Step 1" (now labelled in the diagram above), the internal object model would still be JToken. "Step 2" we swap out the JToken implementation for a POCO implementation, "Step 3" we move the JToken API out to another package. |
Don't you think consumers of this library would appreciate having a fully typed |
I suspect that some would. What I was looking for was the lowest-common denominator to start from, then layer such things (typed input, Newtonsoft objects, System.Json objects on top). There are certainly use cases where I want to receive a raw string of JSON (e.g. sent to an API) and expand it before doing anything further. To be honest, at this very early stage of refactoring, my main design goal was the simplest way to a dependency free API. Strings seemed like a good starting point, which we could refine later. You may be right that an object model should serve as input to the raw API interface. |
I agree that it should be possible to get to the original |
Ok, is it that you want the outward facing API to take an object model for the whole graph being passed in? Or are you simply wanting the option to pass in a typed The pro/con with a opinionated type for For the document other than the |
Yes, something like public class Context : IList<Dictionary<string, object>>
{
private readonly string json;
public override string ToString()
{
return this.json;
}
} |
Ok I'll use that as a guide for the internal representation, but I still wonder whether accepting a string of JSON might be a less opinionated way to provide the basic external API - although we could provide a mapping layer I suppose. |
Is there any downside to not being able to process JSON-LD documents which contain keywords which we don't yet recognise? I'm not sure what the rules are for unrecognised property names starting with an @ symbol. |
As the matter of what's fed into the outer layer of the application, it will of course be a As an architectural overview, this is how it would look like: If we detail the process, it looks like this: |
As long as we expose everything "loosely typed" in a dictionary, I think we're good. |
in "Infrastructure" above, do you propose that lies outside of this library? (and also what is your diagramming tool :) ) |
No, I'm thinking one project for "Core" and one or more projects defined as "Infrastructure", all in the same solution. |
I'm not particular about .sln solutions. I think what's critical is the package structure for nuget (i.e. do people have to install all the Infrastructure stuff, or can they pick and choose). |
Perhaps to start moving us in the right direction, I'll move the string-based API into a distinct project (but, for now, within the same solution) with the intent that the Newtownsoft one will move out to its own project once we're happy with the overall direction (and provide users with a smooth upgrade path) |
Agreed! Sounds good. 👍 |
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>netcoreapp2.1</TargetFramework> |
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.
would prefer netstandard, but for now not wanting to change the core test project which is netcoreapp
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.
Yeah, let's have netstandard
as a goal, but we'll get there when we'll get there.
@@ -1,7 +1,7 @@ | |||
| |||
Microsoft Visual Studio Solution File, Format Version 12.00 | |||
# Visual Studio 15 | |||
VisualStudioVersion = 15.0.28010.2026 | |||
# Visual Studio Version 16 |
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 hate sln files
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.
Who doesn't. 😄
I've just noticed that we (very worryingly) have both A few of our unit tests are against Wondering if I should park this refactoring for a while and instead go make the tests a bit less of a shambles. |
I suppose we could start by deciding to deprecate a large amount of this surface area? |
👍 to that. |
Can I ask what you use for your fab diagrams @asbjornu ? |
Thanks for thinking they are fab! 😊 I use Lucidchart. |
ah i use that too but usually for flowcharts. Will have to look at it a bit more. Nice palette etc. |
This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions. |
No description provided.