[csharp][netcore-httpclient] Refactor of constructors: removed obsolete attribute

[lucamazzanti]

Removed the obsolete attribute on constructors without the HttpClient parameter.
See last comments in PR #9145.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master, 5.1.x, 6.0.x
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

cc @mandrean (2017/08) @frankyjuang (2019/09) @shibayan (2020/02) @Blackclaws (2021/03)

[Blackclaws]

Lets best continue the discussion over here then.

I am against removing the obsolete Attribute again. The reason being that as it stands right now people WILL assume that just building the API accessor class is fine, if they get an obsolete warning (not an error) they are at least aware that what they are doing is not really what they should be doing.

I understand the sentiment of wanting to relieve the burden of people that just "want to use it in a simple project" however those people could just view the message on be like: "Ok doesn't apply to me"

[lucamazzanti]

I let that choice to you and @wing328. In case this is not the expected I will close the PR.

For my point of view, in a .net core project, I want the HttpClient exposed because I want to manage it correctly.
I understand if someone create a short life cycle project it is not useful, so maybe removing the ctor in the future marking it as obsolete is too much.

But if someone put the parameterless constructor for example in a controller, the issue start consuming sockets giving real pressure on the environment.

As I said starting the issue a month ago, we have stressed a lot our production environment from the inside, moving a project from net framework to net core, and it was really difficult to understand the change of behavior.
So good to me, we have the httpclient version now, but people must be aware of this client lifecycle.

[wing328]

I am against removing the obsolete Attribute again. The reason being that as it stands right now people WILL assume that just building the API accessor class is fine, if they get an obsolete warning (not an error) they are at least aware that what they are doing is not really what they should be doing.

From what I understand, we all agree this constructor will NOT be removed in the future releases. Then the next question is whether obsolete is the right word since from what I know, obsolete means the method will be removed in the future releases (which is not true in this case).

however those people could just view the message on be like: "Ok doesn't apply to me"

Agreed. Have been working on this project for a while and this happens from time to time that users ignore warnings/messages or whatever we put down in the documentation/docstring/code comments, etc. Even though we mark it as obsolete, users may still ignore it.

What about updating the following lines:

https://github.com/OpenAPITools/openapi-generator/pull/9373/files#diff-08fdb1a8e47bd4871625bae8d784ebea52c508c1135520e6d255bc67681989b2L111

https://github.com/OpenAPITools/openapi-generator/pull/9373/files#diff-08fdb1a8e47bd4871625bae8d784ebea52c508c1135520e6d255bc67681989b2L122

https://github.com/OpenAPITools/openapi-generator/pull/9373/files#diff-08fdb1a8e47bd4871625bae8d784ebea52c508c1135520e6d255bc67681989b2L143

with additional information: Constructors without HttpClient have non-trivial drawbacks. Check README.md for details. ?

[lucamazzanti]

I agree the obsolete is intended for the removal not a warning. We can use the remarks element in comment.

[Blackclaws]

I am against removing the obsolete Attribute again. The reason being that as it stands right now people WILL assume that just building the API accessor class is fine, if they get an obsolete warning (not an error) they are at least aware that what they are doing is not really what they should be doing.

From what I understand, we all agree this constructor will NOT be removed in the future releases. Then the next question is whether obsolete is the right word since from what I know, obsolete means the method will be removed in the future releases (which is not true in this case).

I think obsolete is still semantically correct. It exists purely for backwards compatibility purposes (or quick testing) and while it won't be removed it definitely is obsolete for usage purposes.

[wing328]

Another point for debate is how the users (developers) interpret the word "obsolete".

What about going with my suggestion in #9373 (comment) and revise later baed on users' feedback/question?

[Blackclaws]

Obsolete means that it has been superseded by a different method. The problem is that C# doesn't offer a different attribute that would give you a compiler warning. So we don't really have a choice if we want to give any warning inside the code that doesn't rely on people reading the readme in advance.

I'm fine with removing the "and are considered deprecated" as its not necessarily true that they will be gone at some point.

[wing328]

I've pushed an update (aa0adcd) to include a clickable link in the tooltip, e.g.

Please review to see if this looks better to you.

[Blackclaws]

I think having that clickable link there is definitely a good thing. I'm not sure people will notice it more if you write IMPORTANT. As I usually don't read the description for methods that I have used before or that I don't expect have any relevance to me.

How about putting the part after IMPORTANT into the Obsolete Attribute as a quite clear warning.

[wing328]

I'm not sure people will notice it more if you write IMPORTANT.

In the worst case nothing (including obsolete) matters to the users who skip everything.

Here is the official definition of obsolete: Marks the program elements that are no longer in use

Ref: https://docs.microsoft.com/en-us/dotnet/api/system.obsoleteattribute?view=net-5.0

But I don't think the constructor in questions fits the definition.

[Blackclaws]

As I said obsolete doesn't quite fit the bill. I thought it was probably the only easy way to make a warning available to the consumers. However we might just want to use a

#warning This constructor has non trivial drawbacks. Consult Readme.md for details

inside the constructor code that should also trigger a compiler warning which is much more visible.

[wing328]

Pushed another commit to update the samples to use HttpClient, HttpClientHandler. Updated tests with HttpClient, HttpClientHandler.

Please have a look when you guys have time.

[lucamazzanti]

For me it's ok the remark and think about the obsolete in the near future based on feedback.
If someone of you can help me on #9389 before the release, it is more impacting than the httpClient issue.

[wing328]

@lucamazzanti agreed. Let's go with this. We can always revise later as httpclient support in the C# netcore client generator is still beta and may subject to breaking changes in the future without further notice.