-
Notifications
You must be signed in to change notification settings - Fork 0
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
reduce verbosity of rule definitions #2
base: master
Are you sure you want to change the base?
Conversation
@tophf I was hoping to get to it this weekend. Sorry for not getting back to it for so long. |
Before addressing the PR, I'd to address supporting the single/double quoted strings in places where the syntax is known like GM_addStyle and /* css */ etc. I shied away from supporting it initially mainly to reduce complexity or providing less than half-baked support. I'll use the current (limited) support for single/double quoted strings for HTML as motivation.
So if we support single/double quoted string for CSS using similar approach,
GM_addStyle(".class1 { font-size: " + getZoomRatio() + "; } .class2 { border: " +
getBorderWidth() " solid red; }"); The rules would need to recognize:
All in all, I feel we might end up requiring a lot of complexity to handle different cases, and the support might still be incomplete. I feel syntax highlighting single / doubted quoted strings is less useful in practice as well (other than aesthetically being more pleasing and uniform). E.g., the highlighter would be less helpful in recognizing syntax issues, e.g., missing ending |
For the PR, I'll address the compact syntax in this comment, and other minor ones in another one to make things clearer. With the more compact syntax, I feel the rules are easier to read, but changing rules might be more error prone, as it is less clear on what is allowed and what is not allowed. E.g., in the css rule, // GM_addStyle(`css-string`);
[
...
['quasi', { // if it's a string template
next: 'css-in',
mode: cssMode,
onMatch: prepReparseStringTemplateInLocalMode,
}],
[isEndBacktick, {
id: 'css-in',
... In the @tophf Given there is some distance in time (from the PR you submitted initially a few months back), what do you feel about the syntax now? Do you feel it is as maintainable as it was back then, when everything was fresh in your mind? If so, let's proceed to merge the PR (pending a few minor things). |
Only 1 other comment for the PR:
|
|
That'd work too. It's limited, but in a way that developers could easily understand. Bear in mind that html is largely supported in practice due to the support of automatic highlighting single/double quoted string as html as long as it starts with a tag. The main missing piece is css E.g., the following code snippets are highlighted as: // automatic highlight for HTML single/double-quoted string, as long as the string begin with a tag
let someMsg = '<span class="msg" style="padding: 4px;">hello</span>';
// no highlight for HTML single/double-quoted string when the string does not begin with a tag
// a html hint will not help either.
let someMsg2 = 'Foobar <span class="msg" style="padding: 4px;">hello</span>';
let someMsg3 = /* html*/ 'Foobar <span class="msg" style="padding: 4px;">hello</span>';
// no highlight for CSS single/double-quoted string
let someCSS = /* css */ ".msg { border: 1px; }";
GM_addStyle(".msg { border: 1px; }"); |
@orionlee, I've removed the fuzziness from definitions so only |
Might be even more compact than your initial syntax, the goal is to make the flow/sequence of rules more obvious.
The reason I did this small refactor is that I want to try to support single/double quoted strings in places where the syntax is known like GM_addStyle and
/* css */
etc. but now I wonder maybe there was a reason you didn't add it in the first place?@orionlee, ping!