tintoy / msbuild-project-tools-server

Language server for MSBuild intellisense (including PackageReference completion).
MIT License
58 stars 16 forks source link

We don't need documentation mode to be on #91

Open DoctorKrolic opened 10 months ago

DoctorKrolic commented 10 months ago

This line enables "documentation mode" for our source folder: https://github.com/tintoy/msbuild-project-tools-server/blob/3ae2ece400a46609af76e18579bcb835f28d4f4f/src/Directory.Build.props#L5 This means that C# warns on every undocumented public member of any type in our main codebase. While I agree that this can be helpful for library authors I am very much against that thing in our case since this language server is not a library, but a standalone app. If something is unclear we can always go-to-def, search symbol or use other IDE feature, all of which is not directly available when you reference a library in your project (and this is where docs come in handy. You cannot see how e.g. a parameter is used in external code, but you have a little doc, explaining what it is for). At the same time keeping this docs at least partially sane is a maintance burden and in many cases thy end up being either totally unuseful, e.g. "a logger is a logger" or "a cancellation token can be used to cancel the operation", "Main is a program entry point" and so on. At the same time these docs take vertical space, so whenever I have some code opened ~50% of my screen space is just filled with docs, which, as I showed above, are not that much useful. So in the end I think we should remove this forcing of documentation and clean up the codebase from most of the docs. This doesn't mean that I want to remove all code comments entirely, ofc. if something is not straight obvious we might need to add a comment or doc, but this should happen only when necessary, so if I see a doc comment I know this is something important and not just a bloatware, which is here only because the compiler forces to do so

tintoy commented 10 months ago

Can you give me a day or two to think it through? I don’t wan to be dogmatic about this, but it’s something I have believed in quite strongly for many years now and I want to work out exactly what’s driving that belief 🙂

To me, doc comments are a way of ensuring that the purpose of each member is considered when adding it. I wouldn’t document every variable but members have a scope that doesn’t always fit on a single page/screen and I find it helpful when keeping track of what that scope is and how it fits into it. Having said that, it’s true that some comments wind up being almost ritualistic (and I tend to regard those as a necessary evil) - in my experience, when given a choice to only document some things people almost always choose not to document them at all. And I prefer to prioritise the productivity of whoever has to look at the code next time.

Some of that stems from my own limitations though (I have some issues around attention and memory), and perhaps there is more room for compromise than I had considered up until now.

tintoy commented 10 months ago

Ok, so I’ve spent some time examining my thoughts on this, and here they are:

I’m fine with the idea that we don’t have to document all members everywhere (or even most of them depending on definitions vs justifications) but I am totally against the idea we shouldn’t have doc comments at all

I’m ok with turning off the doc mode so you don’t have to add comments to things. I’m not really happy with just removing existing doc comments but I guess if you can describe the criteria for when a given doc comment should be removed I’m open to discussing it 🙂

For the record, I believe that maintaining comments is part of the cost of code (not an overhead). But like anything else it does have a point beyond which there are diminishing returns (such as documenting constructors that are only ever used for dependency injection).

Anyway, tldr - we can turn off the doc generation and, if you want to describe cases where documentation is not justified, then we can use that as a starting point for gradually removing them until we are both equally comfortable with it.

How does that sound?