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

Config for force transpile brs file #573

Merged

Conversation

justinMBullard
Copy link
Contributor

Configuration setting added to BsConfig to allow for BRS file to be transpiled like BS file.

@TwitchBronBron
Copy link
Member

Should we have separate flags for "run the brs code through the transpiler" and "allow brighterscript features within .brs files"?

The transpileBrs option sounds like the first item, and doesn't make it immediately evident that you can now use brighterscript features within .brs files.

This also doesn't handle whether xml files should be transpiled or not.

Should we have two flags instead, that represent that following actions?

  • run brs and xml files through the transpiler (not sure why you'd need to do that though on its own)?
  • allow BrighterScript code inside .brs files

@justinMBullard
Copy link
Contributor Author

Should we have separate flags for "run the brs code through the transpiler" and "allow brighterscript features within .brs files"?

The transpileBrs option sounds like the first item, and doesn't make it immediately evident that you can now use brighterscript features within .brs files.

This also doesn't handle whether xml files should be transpiled or not.

Should we have two flags instead, that represent that following actions?

* run brs and xml files through the transpiler (not sure why you'd need to do that though on its own)?

* allow BrighterScript code inside .brs files

I think just one setting to enable BrighterScript code in .brs files. My intent is to enable legacy code bases to work with BrighterScript. When I evaluate the ability to do traspiling I can already (and am already) doing some of that in existing compiler plug-in model.

I am asking myself...What does transiling files mean to the BrighterScript Compiler if BrighterScript is not enabled? The answer I give myself is nothing. But I am new to the project and don't have a historical background.

@TwitchBronBron Should i change transpileBrs to brighterScriptInBrs?

@TwitchBronBron
Copy link
Member

Currently, brightscript files that have zero AST changes will be copied as-is, and won't run through the transpiler. So in my head, that's a separate thing from "allow brighterscript features in brightscript files". However, that's just a product of the current design. Also, plugins that modify the AST would also need to transpile their files even if not using brighterscript syntax enhancements. Perhaps you're right, there is no real difference from an end-user perspective?

I've been thinking maybe we should name this so that it's more clear that you're allowing brighterscript features to be written within the .brs file extension. Maybe something like

  • allowBsInBrs
  • allowBrighterScriptInBrightScript
  • treatBrsAsBs

@elsassph
Copy link
Contributor

elsassph commented May 2, 2022

I kind of like transpileBrs, but it should be clear what it means.

@justinMBullard
Copy link
Contributor Author

@TwitchBronBron @elsassph
How about allowBrighterScriptInBrs or transpileBrsWithBrighterScript?

@justinMBullard
Copy link
Contributor Author

@TwitchBronBron @elsassph Any decision on the name of the setting?

@TwitchBronBron
Copy link
Member

I'm leaning towards allowBrighterScriptInBrightScript. @elsassph @chrisdp any objections? I'd like to get this finalized early this week. It's waited long enough (I'm sorry @justinMBullard for the delay).

@justinMBullard
Copy link
Contributor Author

I'm leaning towards allowBrighterScriptInBrightScript. @elsassph @chrisdp any objections? I'd like to get this finalized early this week. It's waited long enough (I'm sorry @justinMBullard for the delay).

@TwitchBronBron Should I make name change?

@TwitchBronBron
Copy link
Member

Yeah, go ahead and make the change. Sorry for being so slow on this. Let's go with allowBrighterScriptInBrightScript.

@justinMBullard
Copy link
Contributor Author

justinMBullard commented May 19, 2022

Yeah, go ahead and make the change. Sorry for being so slow on this. Let's go with allowBrighterScriptInBrightScript.
@TwitchBronBron I made the change and also Rename transpile-br to allow-brighterscript-in-brightscript.

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Looks great. Just a few small suggestions.

benchmarks/targets/transpile-brs.js Outdated Show resolved Hide resolved
src/BsConfig.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
@TwitchBronBron TwitchBronBron merged commit fc1f961 into rokucommunity:master May 23, 2022
TwitchBronBron added a commit that referenced this pull request May 23, 2022
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.

None yet

3 participants