Make unittest of rules works #4
Conversation
kunaltyagi
dismissed
their stale review
Didn't realize that Reviews are not per commit, but per pull request
|
|
||
| token = lexer.GetNextTokenSkipWhiteSpace() | ||
|
|
||
| error(lexer) |
There was a problem hiding this comment.
Irrelevant line? (L72: error(lexer)) since while True: is never broken from
There was a problem hiding this comment.
Oh, yes.
I will remove L72.
| @@ -44,13 +44,13 @@ def RunRule(lexer, fullName, decl, contextStack, context) : | |||
| if ext == ".h" : | |||
| upperBlock = contextStack.SigPeek() | |||
| # modification to make doxygen compulsory for even private functions | |||
| if upperBlock is None: # != None: and upperBlock.type == "CLASS_BLOCK" and upperBlock.additional == "PRIVATE": | |||
| if upperBlock is not None and upperBlock.type == "CLASS_BLOCK" and upperBlock.additional == "PRIVATE": | |||
There was a problem hiding this comment.
Won't this result in failure in the test case where the function is private? (I'm unable to run tests on my mobile 😄 )
There was a problem hiding this comment.
Test 4 has private function without comments, and the test passes(well, in my environment :P).
I think it would be nice that you can run the tests as double check.
There was a problem hiding this comment.
Oh, I misread the description in file header, because the private function without comment is put in the "Good" block.
So doxygen comments are also required for private function, right?
There was a problem hiding this comment.
Yes, that is the intent. All class members (public, private or protected) and functions should be documented.
| @@ -107,7 +107,7 @@ class A { | |||
| void ~A(); | |||
| } | |||
| """) | |||
| assert CheckErrorContent(__name__) | |||
| assert CheckErrorContent(__name__) | |||
| def test4(self): | |||
| self.Analyze("thisfile.h", | |||
| """ | |||
There was a problem hiding this comment.
This test should give an error (since B and C are not documented)??
| """ | ||
| class J { | ||
| protected : | ||
| /// HELLO |
There was a problem hiding this comment.
It would be nice if the file introduction contained the fact that /// comments are also not valid. Tests 9-12 all are based on this.
There was a problem hiding this comment.
I will refine the file introduction!
|
@chuck-lee Cheers! Apart from a few points I've mentioned above, I see no issue in merging this pull (as soon as I can run the tests). |
|
@kunaltyagi Thanks for your feedbacks, I have updated my patch! |
No description provided.