Open vvdb-architecture opened 10 months ago
@paulomorgado are these regressions of your latest changes?
_baseUrl has been changed here: https://github.com/RicoSuter/NSwag/pull/4541/files
can you fix this?
Do we have a unit test for this? If not, where/how can it be added?
Should we be concerned about these warnings in the generated code?
It's fine to just get rid of the warning with the null forgiving operator. But only if we are sure that it won't be null
on first use. And, in that case, we can just ignore the warning.
v13 always initialized _baseUrl
with "{{ BaseUrl }}"
.
If I understand @RicoSuter well, the the pull request he refers to and became part of v14 simply removed that initialization. Hence the warning appears.
As for the concern for warnings, strictly speaking there is no concern since "they are only warnings". But if you work for an employer that strictly enforces "zero warnings" at the quality gate, and wants you to explain why some warnings are explicitly disabled instead of writing the code correctly in the first place, I would not classify them as "concern", but more of a "minor bother". Like a mosquito during summer teasing you.
You should never ignore warnings as they are hinting to possible errors. You have 3 options:
default!
is not an option because it just masks the warningstring?
when nullable reference types are enabledYou should never ignore warnings as they are hinting to possible errors. You have 3 options:
- Initialize the field correctly,
default!
is not an option because it just masks the warning- Define the field as
string?
when nullable reference types are enabled- Disable nullable reference types for the project owning the field
@ViRuSTriNiTy,
Ignoring errors in non-generated code is always my advise.
None of these options are viable options.
#pargma
or initializing with the default value for the type asserting that it's fine is a perfectly good option.string?
would require a null
check on every use. That would be adding unnecessary complexity.v13 always initialized
_baseUrl
with"{{ BaseUrl }}"
. If I understand @RicoSuter well, the the pull request he refers to and became part of v14 simply removed that initialization. Hence the warning appears.As for the concern for warnings, strictly speaking there is no concern since "they are only warnings". But if you work for an employer that strictly enforces "zero warnings" at the quality gate, and wants you to explain why some warnings are explicitly disabled instead of writing the code correctly in the first place, I would not classify them as "concern", but more of a "minor bother". Like a mosquito during summer teasing you.
@vvdb-architecture,
That field is not directly accessed anymore and its value is initialized in the constructor via the BaseUrl
property.
I understand why you are running into an issue here and I can relate to it, although generated code should be checked for in the context of the generator and not the generated code. In fact, like you just did by reporting it here and not by checking generated code.
I'll add a #pragma
with a comment to disable the warning at that line.
Added
#pragma warning disable 8618 // Set by constructor via BaseUrl property
private string _baseUrl;
#pragma restore disable 8618 // Set by constructor via BaseUrl property
to PR #4634.
v13 always initialized
_baseUrl
with"{{ BaseUrl }}"
. If I understand @RicoSuter well, the the pull request he refers to and became part of v14 simply removed that initialization. Hence the warning appears.As for the concern for warnings, strictly speaking there is no concern since "they are only warnings". But if you work for an employer that strictly enforces "zero warnings" at the quality gate, and wants you to explain why some warnings are explicitly disabled instead of writing the code correctly in the first place, I would not classify them as "concern", but more of a "minor bother". Like a mosquito during summer teasing you.
I'm always concerned about warnings, because they hint to possible run-time issues.
It's an illusion to think one can enforce whatever policies on code you don't control. If this was a library or a tool generating IL, that would not be source checked. Or would it be?
What would be the way of "writing the code correctly" in this case?
Added
#pragma warning disable 8618 // Set by constructor via BaseUrl property private string _baseUrl; #pragma restore disable 8618 // Set by constructor via BaseUrl property
to PR #4634.
This does not solve the warning because the warning is raised on the constructor, not on the property. Additionally this prevents users from suppressing this warning (and others) using a custom file header template as well.
In general it is better to suppress such warnings using the null forgiving operator rather than supressing. This code does the same trick and is less verbose.
private string _baseUrl = null!;
This does not solve the warning because the warning is raised on the constructor, not on the property. Additionally this prevents users from suppressing this warning (and others) using a custom file header template as well.
@klemmchr, can you provide practical examples of this?
This does not solve the warning because the warning is raised on the constructor, not on the property. Additionally this prevents users from suppressing this warning (and others) using a custom file header template as well.
@klemmchr, can you provide practical examples of this?
We are setting TreatWarningsAsErrors
during build so we need to suppress every warning in the generated client. In v13 the client had nullability warnings that we disabled using a custom file header template. However, this is just a dirty fix. A generated client should never ever produce any warnings or errors. They are not useful for the user because the user has literally no impact on it.
After upgrading our .NET 6 project from v13 to v14, we notice two things:
(1) Issue #4467 is still valid. The warnings:
... are still issued because the methods
WriteJson
andReadJson
in `Newtonsoft.Json.JsonConverter are defined as:... whereas in the generated code, the methods are overriden with the following signature:
(2) All client types generate the following warning in their constructor:
This is because the field
_baseUrl
was defined as follows in v13:... whereas in v14, it's just:
But since the constructor doesn't reference it, a warning is issued since it's a non-nullable reference. The removal of the initialization string was a good move since it was confusing. To get rid of the error, one can use a "null forgiving operator":
This will get rid of the warning.