Fix packing readme and analysis recommendations

[Shane32]

On NuGet.org, the readme file is not appearing. This is because the PackageReadmeFile did not get set while building. This is because the condition IsPackable == true does not work if IsPackable is not explicitly set, even though when it is not set, it is implied to be true for the purposes of packing and within ItemGroups. This also caused the analysis to be disabled.

The analysis mode revealed that some xml comments on some old code was missing.

I ran the pack locally and examined the output to be sure it built correctly this time.

It is not critical to release 7.0.1, as there are no breaking changes and the 7.0.0 code should work just fine. Nevertheless, we can release 7.0.1 if you think we should @sungam3r , before everyone downloads it. Or, we could wait a week or two to see if there are any other issues that need to be addressed.

[sungam3r]

Sad news. Does this mean that other projects also have this problem? I see readme in graphql-dotner but not parser 🤔 . Looks a bit weird. How about just move property out of Condition?

  <PropertyGroup>
    <PackageReadmeFile>README.md</PackageReadmeFile>
  </PropertyGroup>

The same for AnalysisMode. I understand it requires a bit more work but it should not be difficult. I propose to add CA1707 into NoWarn in Tests.props . After that I see only ~60 issues to fix 😉 . I think this is better than jumping over different order of conditions in MSBuild.

[codecov-commenter]

Codecov Report

Merging #897 (3a97db9) into master (57ebcd8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #897   +/-   ##
=======================================
  Coverage   95.17%   95.17%           
=======================================
  Files          42       42           
  Lines        1991     1991           
  Branches      336      336           
=======================================
  Hits         1895     1895           
  Misses         55       55           
  Partials       41       41           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Shane32]

Does this mean that other projects also have this problem?

Some of my other projects certainly did.

The same for AnalysisMode.

I really don't like the analyzer running for tests. It's very annoying, when it nit-picks at stuff that really doesn't make a difference, often causing test code that is inappropriate or harder to read than before. Here's some examples that annoy me for tests:

1. async _ => true vs Task.FromResult(true)
2. Methods containing one line of executable line of code vs => syntax.
3. Misc naming conventions, which are often not appropriate for tests
4. Unreferenced methods warnings (necessary for tests)
5. Warnings that methods may be marked as static (necessary for tests)
6. Warnings about unnecessary using statements (which are still a problem even without the analyzer)
7. Warnings about one-line methods

I just looked at the 60 other warnings besides CA1707. There's only one warning that makes any sense for tests -- the int.Parse culture warnings. The rest do not apply. I definitely don't want the analyzer to run for tests. Sample code, maybe, but not tests.

Note that item 2 above is a particular problem for sample code; example below:

// -- this will not compile --
services.AddGraphQL(b => b
    .AddUserContextBuilder(httpContext =>
    {
        // you may add any data from the user context, like the value
        // of httpContext.User or Environment.IsDevelopment() or whatever you like
        return new MyUserContext(httpContext.User, Environment.IsDevelopment()));
    })
);

// -- .NET complains and wants this instead, which can be harder to read and does not leave room for comments --
services.AddGraphQL(b => b
    .AddUserContextBuilder(httpContext => new MyUserContext(httpContext.User, Environment.IsDevelopment()))
);

I suggest we merge this to fix the analyzer and readme as it was intended to be. If you want to create a subsequent PR with additional changes to the build process, I'll review it at that time.

[sungam3r]

OK. Then I propose to disable all the rules that are not suitable for tests while turning the analysis on.