apache / pulsar-dotpulsar

The official .NET client library for Apache Pulsar
https://pulsar.apache.org/
Apache License 2.0
234 stars 60 forks source link

Stop duplicate code comments for code review. #181

Closed entvex closed 11 months ago

entvex commented 11 months ago

Description

When reviewing a pull request, duplicated errors pollute the code view unnecessarily.

Example image

When reviewing code. We just want a clean view to do it.

entvex commented 11 months ago

We may still want to verify the docs syntax are valid?

Yes, I understand the intention. But there must be another way to do it, so the code reviewers don't have to see all these duplicates.

Another approach could be if we spun this code into a separate GHA that also triggers on push, like the action it is in now. But do you know if that would stop the duplicates, or would they still show up in the review?

Or we could verify the syntax when creating a release (the new GHA, which I am working on).

entvex commented 11 months ago

Today I have tried to do as I suggested and spun the docfx code away from the ci-unit.yaml and into another GHA named doc-syntax-check.yaml. But it sadly didn't fix the issue, and therefore the duplicated items are still an issue. image

I still think we should remove the docfx from CI, so we don't see duplicates when reviewing code.

entvex commented 11 months ago

Replace dotnet build -p:TargetFramework=net7.0 with dotnet build -p:TargetFramework=net7.0 --property WarningLevel=0 should help.

Functionality checks are covered by unit-tests, we run it in doc-tests for prepare infos only.

Okay. So I tried to use the code in your comment, here in my test repo https://github.com/entvex/pulsar-dotpulsar/pull/2/files. And it looks like the normal unit test will make errors if there are any doc XML syntax errors.

Therefor, I really think we only need to run doc test, when we do a release. And therefore it does not need to be part of the CI unit test pipeline.

image Does it make sense ?