New Rule: Check procedure documentation comments #800
Conversation
|
Looks promising! With the Directions EMEA event happening this week, I won’t be able to review it until next week at the earliest. |
| <value>The documentation comment must match the procedure syntax.</value> | ||
| </data> | ||
| <data name="Rule0072CheckProcedureDocumentationCommentFormat" xml:space="preserve"> | ||
| <value>The documentation comment does not match the procedure syntax.</value> |
There was a problem hiding this comment.
It might be good to adjust the diagnostic messages so they better inform about the problem.
This message is very general. Better is to have the message describe that for example the documentation comment references a parameter that does not exist, and the other way around that a parameter is missing documentation.
There was a problem hiding this comment.
While I agree that a specific diagnostic message is always helpful, I also believe we need to weigh the complexity of the rule. Personally, I believe the rule is sufficient in its current form.
BusinessCentral.LinterCop/Design/Rule0072CheckProcedureDocumentationComment.cs
Outdated
Show resolved
Hide resolved
BusinessCentral.LinterCop/Design/Rule0072CheckProcedureDocumentationComment.cs
Outdated
Show resolved
Hide resolved
BusinessCentral.LinterCop/Design/Rule0072CheckProcedureDocumentationComment.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
If I'm not mistaken, we need to isolate very test in a separate AL file.
When one of these three test's don't raise a diagnostic, the test will report success. So we need to split these into three separate AL files.
There was a problem hiding this comment.
I've quickly tested it, and it seems the [|...|] encapsulation makes sure a diagnostic has to be reported there. So even if the two other diagnostics are reported in the file but one is missing, the test will fail.
Though if tests should still best be separated into files i can split them up too.
There was a problem hiding this comment.
Thank you for verifying this! There's no need to split these into separate files, as that wouldn't be necessary.
| <value>The documentation comment must match the procedure syntax.</value> | ||
| </data> | ||
| <data name="Rule0072CheckProcedureDocumentationCommentFormat" xml:space="preserve"> | ||
| <value>The documentation comment does not match the procedure syntax.</value> |
There was a problem hiding this comment.
While I agree that a specific diagnostic message is always helpful, I also believe we need to weigh the complexity of the rule. Personally, I believe the rule is sufficient in its current form.
Co-authored-by: Arthur van de Vondervoort <44637996+Arthurvdv@users.noreply.github.com>
Co-authored-by: Arthur van de Vondervoort <44637996+Arthurvdv@users.noreply.github.com>
Co-authored-by: Arthur van de Vondervoort <44637996+Arthurvdv@users.noreply.github.com>
|
Thank you for proposing the idea and contributing this new rule, very much appreciated! |
Implements a new rule for the discussion #794.
Currently this rule will report a diagnostic on the following cases:
<returns>documentation comment element does exist, but the procedure does not return a value.<returns>documentation comment element does not exist, but the procedure does return a value.<param name="thename">documentation comment element does exist, but the procedure does not have a parameter namedthename.<param name="thename">documentation comment element does not exist, but the procedure does have a parameter namedthename.(The last two cases are then also reported if a parameter name differs between the documentation comment and the actual procedure.)
In documentation comments a
<paramref name="thename"/>element can also be used (see https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-xml-comments). Currently the rule does not check theseparamrefelements for their name, as I have rarely seen them used and checking them would require some additional processing of the documentation comment syntax descendants. But if it is needed it could probably be added to this rule.Please let me know if there is anything left to do or if anything should be changed with this new rule.