-
-
Notifications
You must be signed in to change notification settings - Fork 24
Conversation
Pull Request Test Coverage Report for Build 113
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor remarks, but looks good overall, thanks for your work!
Can you look at the coverage decrease? I just realized the coverage report does not let one see the missing lines, going to fix this.
doxyqml/qmlparser.py
Outdated
@@ -207,12 +212,18 @@ def consume_wo_comments(self): | |||
if not is_comment_token(token): | |||
return token | |||
|
|||
def consume_expecting(self, type, value=None): | |||
def consume_expecting(self, expect_type, value=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since expect_type
can also be a list I would rename it expected_types
.
doxyqml/qmlparser.py
Outdated
if token.type != type: | ||
raise QmlParserError("Expected token of type '%s', got '%s' instead" % (type, token.type), token) | ||
if type(expect_type) is list: | ||
if not (token.type in expect_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be more pythonic:
if not (token.type in expect_type): | |
if token.type not in expected_types: |
/** | ||
* Function with int default parameter | ||
* @param arg A parameter with a defaultvalue | ||
* @return the result | ||
*/ | ||
int intDefaultParameter(int arg); | ||
/** | ||
* Function with string default parameter | ||
* @param arg A parameter with a default value | ||
* @return the result | ||
*/ | ||
string stringDefaultParameter(string arg); | ||
/** | ||
* Function with property as default parameter | ||
* @param arg A parameter with a default value | ||
* @return the result | ||
*/ | ||
int propDefaultParameter(int arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect the produced C++ function declarations would contain the default values. Is this something you plan to do? (not mandatory for now: making the parser not confused with default values is already nice)
eb7b9bf
to
b5d309f
Compare
Looks great, thanks! |
Since Qt5.12, Qt QML module supports ECMAScript 7 (https://doc.qt.io/qt-5/whatsnew512.html). This includes an upgrade to ECMAScript 6 which includes handling of default parameters values (http://es6-features.org/#DefaultParameterValues).
The goal of this pull request is to fix the error raised by doxyqml when parsing QML with default parameters values.