NET6 support, embedded readme

[sungam3r]

No description provided.

[Shane32]

Why are we adding multi-tfm approvals?

[sungam3r]

Because published packages have multiple TFMs. My goal - 100% correct versioning with any combinations of TFMs/ifdefs/csproj compile conditions.

[Shane32]

It’s an unnecessary complexity. We don’t have ifdefs.

[Shane32]

If I’m wrong and we do, that’s one thing, but if we don’t then we should not be cluttering up the code unnecessarily.

[sungam3r]

Let me explain my motivation.

We don’t have ifdefs.

For now yes but we have multiple TFMs and this is already a sufficient condition. This actually means that we have physically different API sets. The code is compiled into independent assemblies and these assemblies are distributed together in one nuget package. These assemblies are now identical. I do not have any desire and the ability to determine this identity manually. Let the test make it for me. In the case of GraphQL.NET I have already seen a precedent when, after adding a new TFM - net6, it took the priority over the already existing TFMs and hid them behind net6-specific approval file.

cluttering up the code unnecessarily

I disagree about cluttering. The test is written in so that it does not generate a single unnecessary file. As soon as something really changes in the public API in any TFM for any reason, it will be visible.

I will emphasize once again - it is done to never admit the invisible divergence of public API in the future.

[Shane32]

It is completely unnecessary. It is unnecessary in GraphQL.NET, and completely pointless here. You changed a method with about 3 lines of code in it to a method with 30 lines of very delicate code in it. I don't want to be the guy trying to fix it the moment that the project file changes or the SDK compiles it into another folder or whatnot -- when there's absolutely no reason for this change. It's not a good idea.

You can put in another PR once there's a REASON for this code. There is no reason now. I don't even know why this project is adding a tfm for net6 at all. It should not be necessary; it's not like the runtime bindings will change or anything.

Let's focus on actually fixing issues and improving the codebase. Not adding pointless tests. For example, right now the project requires a dependency on Newtonsoft.Json even when you're using System.Text.Json. That would be something worth fixing, something that actually makes a difference. Other users are having memory leaks with subscriptions. They honestly don't care two cents about whether the public API changes or not, so long as it works with their code.

[sungam3r]

I described my arguments. From your side I see only a remark that the test code has increased from 3 lines to 30. I can not seriously refer to such an argument, sorry. It is better to have a correct 100-lines code than 10-line code which leads to errors or incorrect interpretation.

when there's absolutely no reason for this change. It's not a good idea.

I persistently try to explain the reasoning. And I showed an example of an error with approvals that has already happened - hiding of one API set behing another. I tried to describe it as much as possible. I have nothing more to add further than what I have already explained.

[sungam3r]

Multi-TFM moved in other PR.

[codecov-commenter]

Codecov Report

Merging #665 (2868d8c) into master (ecc92a3) will increase coverage by 0.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   54.52%   55.02%   +0.50%     
==========================================
  Files          64       64              
  Lines        1592     1592              
  Branches      158      158              
==========================================
+ Hits          868      876       +8     
+ Misses        670      662       -8     
  Partials       54       54              

Impacted Files	Coverage Δ
src/Core/DefaultGraphQLExecuter.cs	88.46% <0.00%> (+7.69%)	⬆️
src/Core/Extensions/ServiceCollectionExtensions.cs	56.52% <0.00%> (+17.39%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecc92a3...2868d8c. Read the comment docs.