Code action callfunc operator refactor #385
Conversation
| //define the function in two files | ||
| const file = program.addOrReplaceFile('components/main.bs', ` | ||
| sub doSomething() | ||
| someComponent.callfunc("doThing") |
There was a problem hiding this comment.
nobody would actually have this code, coz it would not work (it woudl do nothing at run time).
It would have to be someComponent.callfunc("doThing", invalid), which (imho) should become someComponent@.doThing() (which is semantically equivalent).
There was a problem hiding this comment.
While I understand the "standard", I've seen callfunc in the wild with no second parameter. I believe @elsassph also had mentioned they use it like that in their code successfully.
However, I agree, we should refactor someComponent.callfunc("doThing", invalid) to someComponent@.doThing() as well since I think that's what we do behind the scenes in the transpiled BrightScript, so I'll update this PR to support that as well.
There was a problem hiding this comment.
callfunc without argument should be a warning, but I have seen it.
There was a problem hiding this comment.
yeah, guess you guys are right, if there's a default param on the function, then in that case callfunc without invalid will work (I wasn't aware of this or it changed since I got this notion > 2 years ago). Clearly, we can never know if this is the case, so 🤷 I'm not sure if even have a warning here..
|
I've implemented your requested changes. Please review again. |
TwitchBronBron
force-pushed
the
master
branch
from
ebfb3e5
to
e98ca6d
Compare
|
@TwitchBronBron any reason to not merge this after fixing conflicts? |
|
It's probably okay to merge. I wanted to think about the API structure a bit, but it's probably fine. 🫠 |
|
Oh, i remember why. I wanted to think about that callfunc with the second parameter thing. We should chat about this on Tuesday to flush out the details. |
|
Sure. I think the TLDR from me is any change to how we handle the second param/lack there of should be in the V1 release. Also need to check what logging we get if it is missing and can the developers easily self diagnose when there is a miss match. (Mostly notes for my self for when we chat) |
Adds code action in brighterscript files to suggest refactoring a normal callfunc call into using a callfunc operator.
I don't produce any diagnostics for this, as it's not the compiler's job to suggest such a preference-based thing. However, since it's easy enough to provide hints when you click on it, i think that's a fair compromise.
Resolves #240
Notable changes: