Add CS1591 removal pragma

[frblondin]

The goal of this PR is to add pragma instructions that disable & restore CS1591 warnings. Depending on the project options, missing publicly visible documentation can break the compilation.

Here is an example:

partial class ValidatingRecord
#pragma warning disable CS1591 // Missing XML comment for publicly visible type or member
{
	public ValidatingRecord(string Name)
	{
		this.Name = Name;
		Validate();
	}

	...

#pragma warning restore CS1591 // Missing XML comment for publicly visible type or member
}

This question has been discussed and deeply explored in here. Would you have any opinion for this pull request? Do you think that CodeGeneration.Roslyn could manage this as suggested by @amis92 ?

[AArnott]

Interesting. I actually hit this recently myself. But I expect my approach to solve it would be to add the missing xml doc comments to the generated code. Presumably someone who has CS1591 enabled for the overall project does so because they want documentation for all public API. It doesn't matter to the user whether the public API was generated code or not, the net effect is the project fell short of its goal to offer documentation for all of it.

Is adding documentation to the generated code an option for your scenario?

[frblondin]

Is adding documentation to the generated code an option for your scenario?

For the generators I'm writing yes, for sure. I'm using other generators and I could provide a PR to add it.

But still...

• Adding documentation for auto-generated methods such as WithXXX methods in RecordGenerator provides no value. The point of enforcing comments in a project has more value for code written by a human and more especially for methods that are doing non-obvious things that need to be documented
• Adding this pragams don't hurt anyway if the documentation is generated by the code generator. It will just prevent the project compilation to fail in the case where the generator wouldn't generate it

I'd suggest adding these pragmas. Again, doesn't hurt!

[amis92]

I'll paste my suggestion from the mentioned PR:

---

I think this should actually be done as a part of the framework (CodeGeneration.Roslyn). I imagine there would be an assembly attribute that would take params string[] codes and add #pragma warning disable <code> for all given codes in every generated file. That'd also allow to ignore any other warning in generated code files.

So the solution for your problem would be adding a file like AssemblyAttributes.cs with the following contents:

[assembly: GeneratedCodePragmaDisable("CS1591")]

Another approach would be to pass these values as a .csproj properties. Not sure which is best.

[LokiMidgard]

I'll agree with @amis92 that it should be a flag set by the consumer of the generator library in his project.

tl;dr
To disable the warning for all generated code will be good in some use cases, but for others it will be a problem and can introduce bugs.

---

If you have a warning enabled than you normally want to prevent specific problems. In this case I assume the problem you want to prevent, is that the consumer of your project can interact with any method, class, etc. that is not documented. So in this case, if you don't want to comment them they shouldn't be public or you actually want to comment them but the generator does not support it.

Because in those two cases the shortcoming is actually how the generator is implemented, it may be wise to delegate the decision to suppress code warnings to the consumer of the generator. Otherwise the generator could not surface the decision to the consumer or uses the wrong choice.

I would definitely be against turning this warning off for every generated code in the framework without any influence to the writer of the generator or the consumer.

Assume there is a generator that generates public methods and properties and will also document those if an attribute is that that contains the documentation. If now the consumer of the generator has enabled the warning, he wants to be warned that there are any comments missing. And he can add those missing comments through the implementation of the generator. If you globally just deactivate the warning for all generated files, it would prevent the consumer to find the bug that not all methods are documented.

To end this comment, I would not suppress the warning in the default settings. Because if you see the problem you can dig in the documentation and deactivate it. If you don't get a warning and don't now that it is suppressed the consumer assumes all his code is documented, which may not be the case.

[LokiMidgard]

Another solution for this problem, which I admit is a bit hacky, to add support in the framework that the consumer for the library can change the generated code after it is generated. E.g.:

// This will replace or add the provided comment to the mentioned class, property, method, etc...
[assembly: GeneratedCodeFix(
    member:"MyNamespace.MySubnamespace.MyClass.MyMethod",
    comment:"/// <summary>\n    /// MyTest Method\n    /// </summary>")]

// This will change the visibility of the member to the provided value.
[assembly: GeneratedCodeFix(
    member:"MyNamespace.MySubnamespace.MyClass.MyMethod",
    visibility:Visibility.Internal)]

While hacky, this approach would allow the consumer not to ignore the problem but to actually solve it. Either by commenting the code or changing the visibility so the warning will not appear.
Still very hacky 😒

[frblondin]

I understand your point @LokiMidgard . Note that turning the members into internal ones won't fix these warnings, FYI.

What is frustrating is that there is no intermediary solution in a project: I want to enforce developers to comment their code but I also want to use generators to facilitate their lives and prevent errors/anti-pattern. I have only two options: do a PR and cross fingers so that the generator maintainer accepts it, or just don't use the generator as it creates rejected code. In my case - personal call - I think it's acceptable if these auto-generated members are not documented.

I still believe that deciding that some warnings could be ignored should be an option. As you pointed out, it should not be done by default, just an option.

So... a less hacky system could be explicitly adding pragmas on-demand:

// This will add pragma to specific types, or for all types if the type is omitted
[assembly: GeneratedCodeFix(
    pragma:"CS1591 // Missing XML comment for publicly visible type or member",
    type:"MyNamespace.MySubnamespace.MyClass.MyMethod")]

CodeGeneration.Roslyn would then simply add these pragmas as explicitely requested through these attributes.

[amis92]

@frblondin There is actually an intermediate solution: <include> tags. You can decorate the hand-written partial class with this tag, and include a hand-written XML file with all the comments. See https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/include

[frblondin]

This would require a developer to manually comment an auto-generated code. Not really attractive to me - but that's still another solution 😄

[amis92]

I'm debating whether this feature should depend on [assembly: ...] attributes or be configurable via project properties, e.g.

<Project>
   <PropertyGroup>
    <CodeGenerationRoslynPresetPragmaDisable>CS1591</CodeGenerationRoslynPresetPragmas>
   </PropertyGroup>
</Project>

I think Project property is more appropriate, scales up better (Directory.Build.props) and is more configurable.