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

Syntax highlighting for embedded brs in scenegraph #316

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

Conversation

TwitchBronBron
Copy link
Member

This adds syntax highlighting support for embedded brightscript/brighterscript code in CDATA tags within scenegraph xml files.

Before:
image

After:
image

NOTE: this might prevent xml formatters from working properly.

@TwitchBronBron TwitchBronBron force-pushed the embedded-brs-in-scenegraph branch from e9c74a4 to cce39b0 Compare October 28, 2021 12:56
@TwitchBronBron TwitchBronBron force-pushed the embedded-brs-in-scenegraph branch from 6be335e to bb2422d Compare October 28, 2021 12:59
@TwitchBronBron
Copy link
Member Author

@rokucommunity/core-team What are your thoughts on merging this? I think it has the potential to take over any other xml language stuff. Should we hold off until we add xml formatting support?

@georgejecook
Copy link
Contributor

NOTE: this might prevent xml formatters from working properly. is a big no no for me.

most folks I know have coding standards that prohibit embedded brs.
most folks I know also use xml formatters.

@chrisdp
Copy link
Collaborator

chrisdp commented Oct 28, 2021

Do we know if this would break tools like the Redhat xml tools? This is used by developers to get completion support for custom components in their xml.

If there is a chance it could break xml tools I would hold off as developing in this style is definitely not the community norm. If anything a quick fix action or command in xml files with cdata to pull it into its own file might be more interesting to most 🤔

@TwitchBronBron
Copy link
Member Author

I definitely don't want to break existing workflows or plugins, so I'll do some more investigation to determine what the full impacts are.

@elsassph
Copy link
Contributor

I concur with: do not break XML coding experience.

@TwitchBronBron TwitchBronBron added the create-vsix PRs with this tag will trigger a vsix build on vscode-brightscript-language for every push label Jun 30, 2023
@TwitchBronBron TwitchBronBron marked this pull request as ready for review June 30, 2023 18:20
@rokucommunity-bot
Copy link

Hey there! I just built a new version of the vscode extension based on 2ca89c3. You can downloaded the .vsix here and then follow these installation instructions.

@chrisdp
Copy link
Collaborator

chrisdp commented Nov 22, 2024

Looping back to this, what was the reason this could break other tools? Did this effectively change the file type in the editor?

@TwitchBronBron
Copy link
Member Author

Looping back to this, what was the reason this could break other tools? Did this effectively change the file type in the editor?

The problem is that we need to define our own "language" type for SceneGraph files. I believe file formatters are done based on the langauge type, not the file extension. So if you have an XML formatter, this may not work anymore.

I haven't tested it in a while though, perhaps that was wrong.

If it does break formatting, then we will need to add our own xml formatting logic into the project before this can land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-vsix PRs with this tag will trigger a vsix build on vscode-brightscript-language for every push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants