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

reduce verbosity of rule definitions #2

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

reduce verbosity of rule definitions #2

wants to merge 3 commits into from

Conversation

tophf
Copy link
Collaborator

@tophf tophf commented Jan 10, 2021

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!

@tophf tophf requested a review from orionlee January 10, 2021 16:33
@orionlee
Copy link
Owner

@tophf I was hoping to get to it this weekend. Sorry for not getting back to it for so long.

@orionlee
Copy link
Owner

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.

image

  • Typical use cases single/quoted strings are when such strings are concatenated together.
  • Therefore, when the tokenizer passes individual string, it'll only see a fragment of incomplete html, leading to incorrect parsing. E.g., in the above codes, the strings '"></div>', 'plain text <hr>' are not recognized at all.

So if we support single/double quoted string for CSS using similar approach,

  • we'll also have some incomplete / incorrect parsing
  • the rule to support it is not exactly trivial. For HTML, we use a very simple heuristics that recognize strings that being with a tag by a simple regexp match locally. The codes don't really need to consider the context.
  • There is no similar local heuristics for CSS. Consider the folllowing snippets:
  GM_addStyle(".class1 { font-size: " + getZoomRatio() + "; } .class2 { border: "  + 
     getBorderWidth() " solid red; }");

The rules would need to recognize:

  • string beyond the first one, e.g., "; } .class2 { border: ", are still part of GM_addStyle call, and should be in CSS mode.
  • it needs to know when to end it too. In this case, recognize the token ")" at the end as ending the GM_addStyle call.
  • Such bookkeeping might have been done in the javascript mode parser. I don't know the internals of it enough to access it.
  • It also has the similar (to HTML) incomplete parsing problem. E.g., it probably can't recognize " solid red; }" as part of the border: .

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 } bracket, as the css text is typically in multiple string fragments.

@orionlee
Copy link
Owner

orionlee commented May 15, 2021

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 'quasi' check rule, I thought "next: 'css-in'," is optional, as it seems that next is defaulted to the next rule in the list. But it turns out it is required after all: taking it out leads to some exception.

@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).

@orionlee
Copy link
Owner

Only 1 other comment for the PR:

  • There is some small enhancement, of supporting lang=html as a hint (and similar ones for CSS), it'd be great if they are the new syntax is added to the test (in test/javascript-mixed/code-source.txt), which serves as a documentation of sort on what the mode supports and its limitation.

@tophf
Copy link
Collaborator Author

tophf commented May 16, 2021

  1. AFAIK even IDE don't support fragmented nested syntax... but even if they do it sounds so complicated - and the result may still be fragile - that I have a feeling we shouldn't implement it.

    We can support the trivial case: just the first single/double-quoted string. It will completely cover the case of short html/css contained in one string fragment and provide at least partial help in other cases.

  2. Indeed, I negated my own idea and now the syntax is less obvious so I'll think about it more.

@tophf tophf marked this pull request as draft May 16, 2021 03:17
@orionlee
Copy link
Owner

orionlee commented May 16, 2021

We can support the trivial case: just the first single/double-quoted string. It will completely cover the case of short html/css contained in one string fragment and provide at least partial help in other cases.

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; }");

Current result:
image

@tophf tophf marked this pull request as ready for review August 21, 2021 14:19
@tophf
Copy link
Collaborator Author

tophf commented Aug 21, 2021

@orionlee, I've removed the fuzziness from definitions so only Rule is accepted with one convenience: match can be string/RegExp, which produces the match function automatically in makeRules.

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.

2 participants