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

[WIP] Color format checking for bslint #94

Merged
merged 37 commits into from
Sep 28, 2023
Merged

[WIP] Color format checking for bslint #94

merged 37 commits into from
Sep 28, 2023

Conversation

disc7
Copy link
Contributor

@disc7 disc7 commented Jul 27, 2023

  • proposed way to check for color formatting in brs and bs files.

@0xW1sKy
Copy link

0xW1sKy commented Jul 27, 2023

does this also include transparency suffix?

I believe that bs will accept:
"FFFFFF", "0xFFFFFF", or "0xFFFFFF00"

I'd recommend it forces the 0x prefix as well as the transparency suffix 00-FF as part of the linting check.

@disc7
Copy link
Contributor Author

disc7 commented Jul 27, 2023

Not yet. I've got a local branch with alpha values allowed, default alpha allowed, case (upper or lower) and Roku broadcast safe color cert requirement compliance.

Once I know we can determine a range to flag known color styling issues (in brs / bs and xml files - given that there is no color token type in ESlint) will update this to include those.

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.

there is no color token type in ES lint, how do we determine the range to flag line numbers?

Just for clarification: bslint is written as a brighterscript plugin, ESLint is not involved at all.

there is no color token type

There probably will never be a color token type. in brs/bs files, Tokens like TokenKind.StringLiteral, TokenKind.IntegerLiteral represent all of the various formats that colors can be stored as, so those are what you'll want to be inspecting.

However, I'm not really sure how you'll be able to validate anything accurately other than in XML files for known "color" fields on built-in roku nodes.

A better type systep is right around the corner, but we still haven't added support for any of the component types, so you won't know which fields are color type.

src/plugins/codeStyle/index.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
}
},
TemplateStringExpression: e => {
const token = e.quasis[0].expressions[0].token;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all the expressions always string literals?

Also this expects the very first expression to be provided - I suspect it can break in a number of ways:

  • empty template string,
  • complex template like ${value}something.

Probably you shouldn't check any case of template string with zero or more than one expression, and you need to ensure the 1st expression is a string. No?

cc @TwitchBronBron

Copy link
Contributor Author

@disc7 disc7 Sep 24, 2023

Choose a reason for hiding this comment

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

Empty template string as in ``, @elsassph , @philippe-wm ?

@@ -0,0 +1,11 @@
sub init()
color = `0xfffff0FF`
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need more cases, like empty strings, complex templates with variables (at the beginning, end...).

@TwitchBronBron
Copy link
Member

I've just pushed some changes.

  • I think I handled the template string quasi logic correctly now.
  • I simplified the tests by keeping the testable code inline right above the expectations.
  • I moved the color tests into their own describe block.

@elsassph any other concerns?

},
TemplateStringExpression: e => {
if (validateColorStyle) {
for (const quasi of e.quasis.map(x => x.expressions).flat()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO I think the linter should bail if there is more than 1 quasi and if it has more than one expression - we shouldn't try to look for colours in a complex template literal, because that's not a use case for setting UI colours, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fair. I guess we're only validating whole strings, so we should do the same with template strings.

Copy link
Member

Choose a reason for hiding this comment

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

I pushed those changes.

@TwitchBronBron TwitchBronBron merged commit 3995e5f into rokucommunity:master Sep 28, 2023
4 checks passed
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.

6 participants