ci(lint): add checks for variables

[uncenter]

This is some atrocious code and probably isn't worth adding this mess but I thought I'd give linting variables a try.

[uncenter]

You can go to the files tab and scroll all the way to the bottom to see the annotations this adds.

[nekowinston]

We can replace this with JSON.stringify AFAIK, output on my end still looked fine

[uncenter]

My primary concern is that this output won't be understandable since it doesn't match what you see for each variable in the CSS file (i.e. not the @var select ... syntax) since it is the parsed version. Thoughts?

Found mismatch in array elements of property "options" of metadata variable `lightFlavor`:
+ Expected: { "name": "frappe", "label": "Frappé", "value": "frappe" }
- Received: { "name": "frappe", "label": "Frappe", "value": "frappe" }

[nekowinston]

what about just saying

variable `lightFlavor` should be `[ "latte:Latte*", "frappe:Frappé", "macchiato:Macchiato", "mocha:Mocha" ]`

so you actually have something to copy; it'd be a few hardcoded strings but only for light/dark flavors, or flavor, + accent.

Also we should probably default to mauve instead of sapphire since that is what all other UI stuff of big ports defaults to.

[uncenter]

@nekowinston:

what about just saying

variable `lightFlavor` should be `[ "latte:Latte*", "frappe:Frappé", "macchiato:Macchiato", "mocha:Mocha" ]`

so you actually have something to copy; it'd be a few hardcoded strings but only for light/dark flavors, or flavor, + accent.

Hmm, I've given this some thought and take a look at the lint annotations now. I think they are very readable, however I didn't make it "copyable" because often it was very hard to tell what was actually wrong with what you had.

Also we should probably default to mauve instead of sapphire since that is what all other UI stuff of big ports defaults to.

This would probably be a separate follow up PR since I'm just going off of the template but yeah sounds good.

[nekowinston]

@uncenter

Hmm, I've given this some thought and take a look at the lint annotations now. I think they are very readable, however I didn't make it "copyable" because often it was very hard to tell what was actually wrong with what you had.

I just meant if the assertions against lightFlavor/darkFlavor (or flavor for non-syncing themes), and accent fail, we know exactly what kind of string it should say; which is why I said

it'd be a few hardcoded strings

[uncenter]

Yeah I implemented that and it didn't look right. It seems like being able to copy it would be the easiest but it was frustrating because it was difficult to figure out why what you had was wrong.

[nekowinston]

It doesn't matter why its wrong, we literally know what string should be there from the template

[uncenter]

In my opinion understanding why the linter is yelling at you is pretty important.

[nullishamy]

For the boilerplate header, the "why" isn't actually important. It's the same for every single userstyle. For any non-experienced contributors (who are the real targets / likely recipients of these lints), it's important to be concise and precise. With a JSON log, you must manually inspect it, interpret what it means, and apply those changes. This is difficult.

There is little correlation between the JSON output and the original Stylus header, which makes understanding / fixing the problem at hand significantly harder, and will overall increase friction between potential contributors and mantainers.

On the topic of interpretation, without colour the diff is extremely hard to read. This is exacerbated by the fact that this lint will often fail for very subtle copy errors (as you showed in the example).

With the problem source of this lint (copy errors) in mind, it only makes sense to directly inform readers that this is the case.
Here is the message structure that I think effectively communicates this idea:

Invalid boilerplate detected (did you copy it wrong?).
Variable `lightFlavor` should be `@var select lightFlavor "Light Flavor" ["latte:Latte*", "frappe:Frappé", "macchiato:Macchiato", "mocha:Mocha"]`

Lints are for contributors, not maintainers, so we should cater to them.

[uncenter]

For the boilerplate header, the "why" isn't actually important. It's the same for every single userstyle. For any non-experienced contributors (who are the real targets / likely recipients of these lints), it's important to be concise and precise. With a JSON log, you must manually inspect it, interpret what it means, and apply those changes. This is difficult.

There is little correlation between the JSON output and the original Stylus header, which makes understanding / fixing the problem at hand significantly harder, and will overall increase friction between potential contributors and mantainers.

On the topic of interpretation, without colour the diff is extremely hard to read. This is exacerbated by the fact that this lint will often fail for very subtle copy errors (as you showed in the example).

If you look at the current lint annotations you will see that the JSON output is no longer in use.

With the problem source of this lint (copy errors) in mind, it only makes sense to directly inform readers that this is the case. Here is the message structure that I think effectively communicates this idea:

Invalid boilerplate detected (did you copy it wrong?).
Variable `lightFlavor` should be `@var select lightFlavor "Light Flavor" ["latte:Latte*", "frappe:Frappé", "macchiato:Macchiato", "mocha:Mocha"]`

Lints are for contributors, not maintainers, so we should cater to them.

From what I can gather it seems like people prefer to know what they did wrong though. I expect a contributor might compare what the linter suggests to what they have, and it is usually a very subtle difference and likely frustrating as they don't understand what they had wrong - I implemented that as Winston had suggested earlier and that's how I felt ahah.

For context, this is what the lint annotations look as of right now:

Metadata variable `accentColor`: default should be "sapphire" but is "blue"

Metadata variable `lightFlavor`: label of option `frappe` should be "Frappé" but is "Frappe"

Maybe a combination of "suggest what they should have" and "tell them precisely what the issue is" would be best?

Metadata variable `lightFlavor`: label of option `frappe` should be "Frappé" but is "Frappe" and ...
Use `@var select lightFlavor ...` instead.

[nullishamy]

I don't see why we need to be this specific. The contributor does not need to care what is actually wrong, it's a boilerplate. If it were lints in code they wrote, absolutely we should be specific. This is not the case, however.

As for your latter suggestion, I'm not sure why we'd duplicate the information? Above details why they don't need specifics, just the general case.

[uncenter]

I don't see why we need to be this specific. The contributor does not need to care what is actually wrong, it's a boilerplate. If it were lints in code they wrote, absolutely we should be specific. This is not the case, however.

As for your latter suggestion, I'm not sure why we'd duplicate the information? Above details why they don't need specifics, just the general case.

Alright. I've made the changes, please check the lint annotations now and tell me what you think :)

[nullishamy]

I don't see why we need to be this specific. The contributor does not need to care what is actually wrong, it's a boilerplate. If it were lints in code they wrote, absolutely we should be specific. This is not the case, however.
As for your latter suggestion, I'm not sure why we'd duplicate the information? Above details why they don't need specifics, just the general case.

Alright. I've made the changes, please check the lint annotations now and tell me what you think :)

Looks good now I think, just one question there!

[nullishamy]

We can revisit refactoring this to be nicer at some point in the future, but it works, and isn't terribly big, so not a huge deal.

[uncenter]

So now the question is how we resolve the lint errors. Separate follow up PR? Some of the errors are about userstyles that only have a single flavor (instead of both darkFlavor and lightFlavor)... should be an easy fix with media queries?

[sgoudham]

So now the question is how we resolve the lint errors. Separate follow up PR? Some of the errors are about userstyles that only have a single flavor (instead of both darkFlavor and lightFlavor)... should be an easy fix with media queries?

2 questions:

1. Just to confirm my knowledge, they're not technically errors and just warnings and wouldn't cause the PR check to fail right?
2. Is there a valid case here where a userstyle wouldn't want to have both variables set? Or is it the case that its always set?

[uncenter]

1. Just to confirm my knowledge, they're not technically errors and just warnings and wouldn't cause the PR check to fail right?

Yep, but I think it would be confusing to warnings on files unrelated to your own PR lol. But yes, not failing.

2. Is there a valid case here where a userstyle wouldn't want to have both variables set? Or is it the case that its always set?

Uhh I think the consensus is it should always be set. Some people (such as me, occasionally) like to toggle their system theme and have that theme matched everywhere else. It wouldn't be a problem for people who don't change their system theme, and even then they can just set both lightFlavor and darkFlavor to the same theme :p

[sgoudham]

A PR to fix the warnings seems like a good next step after merging this 👍