Add missing docs for OC.Setup #17803
Conversation
There was a problem hiding this comment.
These docs aren't really adding anything useful.
There was a problem hiding this comment.
Having docs is better than nothing :) But I will add extra info
There was a problem hiding this comment.
Not really. Docs that don't provide information beyond what's obvious from the code at a cursory glance, or even just the method signatures, is just noise. This is especially true for implementations of interfaces that nobody will directly use, so there's no advantage for IntelliSense.
And I'm saying this as someone who encourages and likes documentation, and whose team has Readme and interface docs enforced.
There was a problem hiding this comment.
The docs only explain what's obvious from the names already. Add information that goes beyond that.
There was a problem hiding this comment.
Example:
"Contract for handling setup events." should become "Implementations of this interface provide methods that are invoked during different steps of the setup phase."
There was a problem hiding this comment.
The current docs on this interface are still only restating the obvious. If you really want to document it (and this being an internally used simple service I don't think it's beneficial to put any more effort into this), then add information about the why, not the what. The latter is trivial here, the former is what you have to dig into the code for.
Same for SetupUserIdGenerator. That really doesn't need /// XML docs, though.
There was a problem hiding this comment.
Then add information about the why, not the what
I disagree; normally, the docs in APIs let the users understand the type, its properties and methods, and extra remarks if needed. Regarding "Why," this could be explained in web docs or a tutorial
Again having docs even with little details is better than nothing :) that's why almost the APIs are not documented
There was a problem hiding this comment.
Really, it is not.
Here we have an interface called Setup User Id Generator. What's obvious from this, since it's spelled out in the code, is that this is, well, an interface (which if you know C# you understand being a similar concept than a contract in the common sense), that generates user IDs for/during setup. The name and type, in itself, should be clear enough for those with the necessary domain knowledge, and in this case, probably they are. But for those who it's not clear, "Contract that represents a setup user id generator." doesn't help. It just makes interface ISetupUserIdGenerator into an English sentence. Why didn't you also specify in the comment that it's public? Wouldn't then "Public contract that represents a setup user id generator." be better? Or is adding in a comment that's already spelled out in the code useless, and we can agree that spelling out the rest is useless too? Why don't we turn every declaration into an English sentence?
And again, what's not useless, is explaining something that's not obvious from the code, what takes an effort to figure out. This brings value, since it lets others not have to figure it out. If you think that's better in web docs or a tutorial, then I encourage you to work on that instead.
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
|
@Piedone can we merge this? |
|
Did you address my most recent comment? |
|
AFAIK yes, that's why you will find two commits after your latest comment |
|
I checked the code, and it doesn't seem so. Sorry, but I can't allocate any more time to argue for avoiding comments that just state the obvious, so I won't be able to review this PR any further. In its current state I don't approve of it. |
No description provided.