-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add symbol listing for Qml #79
Conversation
2454abe
to
11c72d1
Compare
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 is on the right track. I didn't test it (I wonder if you see the symbols in the GUI yet), but the architecture definitely makes sense.
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.
Overall looks like a reasonable way forward 👍
One additional thing to consider though is that the FunctionSymbol class also runs a few small queries to figure out the names and types of parameters (see the argumentsFromQueryMatch function).
However, that is of course also C++-specific and won't work with QML.
We should try to find a solution for injecting the right behavior there as well.
Also added some minor nitpicks.
11c72d1
to
1cad851
Compare
yes we can |
1cad851
to
548670e
Compare
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.
The separation of concerns looks clean 👍
What I'd like to see addressed before merging:
- Find a way to make
FunctionSymbol::arguments
compatible with Qml.- Currently it's not capturing the individual parameters as
@parameter
, so the FunctionSymbol will always report 0 arguments - The types of the arguments would be extracted incorrectly in the
FunctionSymbol::argumentsFromQueryMatch
function, which would have to also differentiate between languages.
- Currently it's not capturing the individual parameters as
- Add tests for all types of QML symbol extraction. Currently this is entirely untested.
I guess we could also do these in follow-up PRs, but I don't expect much conflict in this part of the codebase at the moment, so it would be preferable to have everything in before merging.
Hm, I also took another look at the ClassSymbol. I guess that also won't work correctly, as it will probably associate all members from all sub-objects into itself. |
548670e
to
e38b6b2
Compare
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.
Going in the right direction, but the testing should be more thorough.
6beb17d
to
d49b3ac
Compare
6495217
to
b2a332d
Compare
for more information, see https://pre-commit.ci
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.
Looks good. We'll want to follow up on this with fixes for FunctionSymbol and ClassSymbol, but a very good start 🥳
symbol listing for Qml added.