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

Fix Rooibos_init injection causing duplicate calls #247

Merged

Conversation

luis-soares-sky
Copy link
Contributor

The rooibos plugin will create a main function if it doesn't exist. And if it does, it will inject a Rooibos_init(...) call as its first statement.

However, the way this is set up creates a duplicate call when both the function and statement already exist in the original source code. This causes problems when you need your project to control when/where it's called (i.e. to support sockets, as seen in #212).

This PR helps by detecting if such a call is already made, and only inject it if it's not found.

walkMode: WalkMode.visitAllRecursive
});
if (!initCall) {
editor.addToArray(mainFunction.func.body.statements, 0, new RawCodeStatement(`Rooibos_init("${this.config?.testSceneName ?? 'RooibosScene'}")`));
Copy link
Contributor Author

@luis-soares-sky luis-soares-sky Jan 5, 2024

Choose a reason for hiding this comment

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

I've considered changing this line to use codegen methods instead of RawCodeStatement, but decided not to in order to keep the PR small and concise. It's something that could be considered in a future refactor if we want more safety.

Suggested change
editor.addToArray(mainFunction.func.body.statements, 0, new RawCodeStatement(`Rooibos_init("${this.config?.testSceneName ?? 'RooibosScene'}")`));
editor.addToArray(mainFunction.func.body.statements, 0, new ExpressionStatement(createCall(
createVariableExpression('rooibos_init'),
[
createVariableExpression(`"${this.config?.testSceneName ?? 'RooibosScene'}"`)
]
)));

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably not a bad idea. @TwitchBronBron and I where looking today to move the modifyAssertions to proper ast transformations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree; good idea, we can revisit in another pr.

@georgejecook georgejecook merged commit fc4065a into rokucommunity:master Jan 7, 2024
4 checks passed
@luis-soares-sky luis-soares-sky deleted the fix/rooibos-init-injection branch January 8, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants