Added Slice Doc-Comment Validation for Operation Only Tags and @see Tags.
#3198
Conversation
| string::size_type n = l.find_first_not_of(ws, tag.size()); | ||
| if (n == string::npos) | ||
| { | ||
| return false; // Malformed line, ignore it. |
There was a problem hiding this comment.
Returning false if the line was empty wasn't good behavior.
It would cause the comment parser to think this was unknown tag, not a malformed tag.
And seeing unknown doc comment tag '@see' is very confusing.
There wasn't even a point to returning false, since all the calling code gracefully handles empty lines anyways.
| unit()->warning(file(), line(), InvalidComment, msg); | ||
| state = StateUnknown; | ||
| } | ||
| else if (!lineText.empty()) |
There was a problem hiding this comment.
Should we emit a warning for empty throws?
There was a problem hiding this comment.
The idea behind this is for users who format their comments like:
@throw AlreadyActivatedException
This exception is thrown if the thing is already activated
Just because the rest of the line is empty, doesn't mean that the section as a whole is empty.
It could just be on the next line down.
There was a problem hiding this comment.
Doesn't line include AlreadyActivatedException in this case?
There was a problem hiding this comment.
Nope, but I see what you were thinking now. lineText only includes the text that comes after the name of the exception, parameter, type, etc.
The function we call is
bool parseCommentLine(const string& l, const string& tag, bool namedTag, string& name, string& doc)
and it has 2 'out parameters' name and doc. In this case name will be the name of the exception, and doc (which is lineText here) is anything that comes after that name (in this case the empty string).
| unit()->warning(file(), line(), InvalidComment, msg); | ||
| state = StateUnknown; | ||
| } | ||
| else if (!lineText.empty()) |
There was a problem hiding this comment.
I think we can have a warning for empty return tag.
| unit()->warning(file(), line(), InvalidComment, msg); | ||
| state = StateUnknown; | ||
| } | ||
| else if (!lineText.empty()) |
There was a problem hiding this comment.
I think it would make sense to emit a warning if lineText is empty.
Do we validate that the @param names, match the operation param names?
There was a problem hiding this comment.
Do we validate that the @param names, match the operation param names?
No, not yet.
I plan to add this validation in the future, but it's lower on my list than finishing the metadata validation.
I also think we should check that @returns is only applied to non-void operations.
This PR further improves the validation of doc-comments.
It implements Ending '@see' Attributes with Periods #1085 - which is about how ending
@seewith periods breaks things.Now, if a user ends
@seewith a period, we issue a warning, and remove the trailing period.We enforce that
@param,@exception,@throws, and@returncan only be placed on operations.If they are placed on anything else, we issue a warning, and throw the tag away (along with it's contents).