SublimeText / PackageDev

Tools to ease the creation of snippets, syntax definitions, etc. for Sublime Text.
MIT License
436 stars 83 forks source link

Trim redundant prefixes of syntax test suggestions. #340

Closed Thom1729 closed 3 years ago

Thom1729 commented 3 years ago

The implementation isn't necessarily final, but feedback would be appreciated.

First, a motivating example. The pipe denotes the cursor, and syntax_test.suggest_scope_suffix is false.

Before:

    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.conditional
//  ^^ keyword.control.conditional.if
//     |

Ordinary result of typing ^:

    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.conditional
//  ^^ keyword.control.conditional.if
//     ^ meta.conditional meta.group punctuation.section.group.begin|

With "syntax_test.suggest_trimmed_prefix": true:

    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.conditional
//  ^^ keyword.control.conditional.if
//     ^ meta.group punctuation.section.group.begin|

The implementation is a first pass, but it's probably mostly correct. Notes:

deathaxe commented 3 years ago

Asume your test cases are located within a meta.block because those rules would apply only there.

{
// <- meta.block

    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.conditional
//  ^^ keyword.control.conditional.if
//     |
}

As meta.block is not of interest in test cases and therefore ommited, your algorithm currently fails in stripping meta.condiational. The result is as if the functionality wasn't present then.

{
    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.conditional
//  ^^ keyword.control.conditional.if
//     ^ meta.conditional meta.group punctuation.section.group.begin|
}

It only works if meta.block is included in the assertion as follows.

{
// <- meta.block

    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.block meta.conditional
//  ^^ keyword.control.conditional.if
//     |
}

The stripping algorithm should therefore lookup e.g. meta.conditioanal and remove all items to its left as well.

Thom1729 commented 3 years ago

It should handle that now. Some examples:

// SYNTAX TEST "Packages/JavaScript/JavaScript.sublime-syntax"

{
    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta
//     ^ meta.block meta.conditional meta.group punctuation.section.group.begin
}

{
    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.block
//     ^ meta.conditional meta.group punctuation.section.group.begin
}

{
    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.block meta
//     ^ meta.conditional meta.group punctuation.section.group.begin
}

{
    if ( true ) { }
//  ^^^^^^^^^^^^^^^ meta.conditional
//     ^ meta.group punctuation.section.group.begin
}

Hopefully the implementation's not too abstruse.

deathaxe commented 3 years ago

The algorithm seems to fail in case of more complicated test selectors such as

//                     ^^^^^^^^^^^^^^^^^^^^^^^ meta.function-call.arguments.scl - meta.path.plc
Thom1729 commented 3 years ago

Can you post the line of code being tested? Also, is that Verilog?

deathaxe commented 3 years ago

It's SCL (Structured Control Language) a subset of ST (Structured Text) used to program PLCs. I am about to create syntax support for it.

The code snippet being tested is:

    #struct.arrayFun[1](execute := #var + 10 );
//  ^^^^^^^ meta.path.plc variable.namespace.struct.plc
//         ^ meta.path.plc punctuation.accessor.dot.plc
//          ^^^^^^^^^^^ meta.path.plc meta.function-call.identifier.scl
//                     ^^^^^^^^^^^^^^^^^^^^^^^ meta.function-call.arguments.scl - meta.path
//                     ^ meta.block.fb.body.plc meta.path.plc meta.function-call.arguments.scl punctuation.section.group.begin.scl

Already got an idea of what's going wrong. Maybe nothing we can fix anyway. Due to an syntax issue the pattern meta.function-call.arguments.scl - meta.path fails to match. I wasn't aware of that detail on the time posting my comment.

FichteFoll commented 3 years ago

Thanks for these changes. They seem to work pretty well for the 99% case and we can do follow-ups for edge cases later, if we need to. I also enabled this feature by default.

Also, sorry about the delay, but I wanted to make sure this works properly before packing up a release.