Skip to content
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

Code action callfunc operator refactor #385

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TwitchBronBron
Copy link
Member

@TwitchBronBron TwitchBronBron commented Apr 9, 2021

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:

  • add codeAction to refactor callfuncExpression to use the callfunc operator.
  • fixed bug in code action utils that wasn't supporting update. Also added delete support.

//define the function in two files
const file = program.addOrReplaceFile('components/main.bs', `
sub doSomething()
someComponent.callfunc("doThing")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 [email protected]() (which is semantically equivalent).

Copy link
Member Author

@TwitchBronBron TwitchBronBron Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 [email protected]() 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callfunc without argument should be a warning, but I have seen it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor

@georgejecook georgejecook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment re: unit test, which I think needs a tweak.

@TwitchBronBron
Copy link
Member Author

I've implemented your requested changes. Please review again.

Copy link
Contributor

@georgejecook georgejecook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know what the implications are for not needing to pass an invalid argument are, or whether to have a warning or not, so I say just go ahead and merge.

@chrisdp
Copy link
Contributor

chrisdp commented Jan 15, 2024

@TwitchBronBron any reason to not merge this after fixing conflicts?

@TwitchBronBron
Copy link
Member Author

It's probably okay to merge. I wanted to think about the API structure a bit, but it's probably fine. 🫠

@TwitchBronBron
Copy link
Member Author

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.

@chrisdp
Copy link
Contributor

chrisdp commented Jan 15, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

provide codeAction in .bs when callfunc can be refactored to callfunc operator
4 participants