Tests for autocompletion | Typst | Page 1

scarlet bobcat Dec 11, 2023, 2:39 PM

#

Integrating support for testing autocompletion into the test runner.

#

@tidal cloak we can discuss here

tidal cloak Dec 11, 2023, 2:43 PM

#

Hey there 🙂

I've summed it up here https://github.com/typst/typst/pull/2912

But the main part of it is, are we okay with this kind of metadata // Autocomplete: explicit, with_label, contains "int","integer", excludes "bool" and there's also a few other questions

#

Should I copy them here?

#

Also things are cleanly separated so if you want multiple smaller PRs it's no problem

scarlet bobcat Dec 11, 2023, 2:45 PM

#

The PR size is okay as-is

scarlet bobcat Dec 11, 2023, 2:52 PM

#

tidal cloak Hey there 🙂 I've summed it up here https://github.com/typst/typst/pull/2912 B...

What is the purpose of // Autocomplete: true | false?

tidal cloak Dec 11, 2023, 2:54 PM

#

Like // Ref: true|false but the main reason why I introduced it is that it would be convenient to have a way to ignore errors when validate_autocomplete is true, because often, when you work on autocompletion the errors and hints don't matter, it would make for clearer test but would be a bit more complex

scarlet bobcat Dec 11, 2023, 2:55 PM

#

Hmm, disabling tests might make sense

#

But Ref: true|false is different because it affects the test result. Whereas autocomplete only has an effect if you actually specify expected autocompletions.

tidal cloak Dec 11, 2023, 2:56 PM

#

Thats fair 🤔

#

What about Hints?

scarlet bobcat Dec 11, 2023, 2:57 PM

#

If you want to split it, we could merge the other improvements more quickly. Otherwise, it can stay as one PR.

tidal cloak Dec 11, 2023, 2:57 PM

#

It's okay for me if it's a bit slower and I don't split it

scarlet bobcat Dec 11, 2023, 2:57 PM

#

Hints and errors should be handled the same. Either we have some way to ignore them (or maybe just ignore errors in the autocomplete line by default) or we just don't.

#

I think generally autocompletion tests should be separate from non-autocompeltion ones

#

So maybe they should be mutually exclusive

#

And autocomplete tests ignore errors

tidal cloak Dec 11, 2023, 2:59 PM

#

I think so too

#

Does it make sense to have access, in autocompletion tests, to explicit (in typst_ide::autocomplete(....., explicit : bool) ?

#

as well as label?

scarlet bobcat Dec 11, 2023, 3:06 PM

#

tidal cloak Does it make sense to have access, in autocompletion tests, to explicit (in `typ...

it is okay to always set explicit: true I think

#

explicit is mostly used to show a few default suggestions in case nothing more specific exists

#

nothing that would come up often in tests

tidal cloak Dec 11, 2023, 3:08 PM

#

Should we always test against Completion::apply? cause I think it makes more sense than label some time but other times it has a weird value {variable}/{other_variable}

scarlet bobcat Dec 11, 2023, 4:01 PM

#

tidal cloak Should we always test against Completion::apply? cause I think it makes more sen...

I think always label is okay for now

tidal cloak Dec 12, 2023, 9:12 AM

#

Design problem !

Currently the second annotation works

// test.typ
// Autocomplete: true
// Ref: false
// Hints: false
---

// Autocomplete contains: `-1` "int", "if conditional"
// #i

--- 

// Autocomplete contains: `-1` "len", "first", "last", "at", "push", "pop", "insert", "remove", "slice", "contains", "find", "position", "range", "filter", "map", "enumerate", "zip", "fold", "sum", "product", "any", "all", "flatten", "rev", "split", "join", "intersperse", "sorted", "dedup", "first", "last", "at", "pop", "push", "insert", "remove", "foozerzer"
#().

but if you uncomment // #i it stops working because the parsing gets messed up with probably?

#

this workaround works

#let i = 0
#{ 
  // Autocomplete contains: `-1` "int", "if conditional"
  i
}

#

but it is a bit painful to require of autocomplete test writers

#

plus.. that's not exactly what you want to test

tidal cloak Dec 12, 2023, 9:37 AM

#

I pushed a working version on the PR, I'm maybe a bit verbose in my reporting/error printing warning but we have a verbose option so if some things are too much I can just put them behind that flag!

#

overall it's working pretty good already, it'll be really simple to add a Autocomplete exclude and to print a bit more when verbose

scarlet bobcat Dec 12, 2023, 10:49 AM

#

tidal cloak Design problem ! Currently the second annotation works ``` // test.typ // Auto...

Uncommenting #i fails the second annotation? That's strange since the individual tests should be unrelated.

#

Or do you mean Autocomplete contains: -1 "int", "if conditional" with second annotation?

#

Even then, I don't understand why it would happen

#

Seems more like a bug than a design problem

tidal cloak Dec 12, 2023, 10:52 AM

#

Are they parsed separately?
No, I mean autocomplete doesn't work correctly for the second annotation if we have #i (errors ) before (parsing problem?)

scarlet bobcat Dec 12, 2023, 10:53 AM

#

They are parsed separately

tidal cloak Dec 12, 2023, 10:53 AM

#

It might be a parsing bug or an autocomplete bug but it's reasonable that autocomplete wouldn't work perfectly when there's an error in the file

#

Ah, that's super weird then

scarlet bobcat Dec 12, 2023, 10:54 AM

#

why would you expect int and if conditional after #i though?

#

it's not valid syntax there

tidal cloak Dec 12, 2023, 10:55 AM

#

Makes it seem like they are parsed together but I'll take a look

If you write #i you will get if and int in the completions suggestions

scarlet bobcat Dec 12, 2023, 10:55 AM

#

Ah, right

tidal cloak Dec 12, 2023, 10:55 AM

#

scarlet bobcat why would you expect `int` and `if conditional` after `#i` though?

plus that test is actually passing by itself :p

scarlet bobcat Dec 12, 2023, 10:55 AM

#

why is -1 in backticks?

tidal cloak Dec 12, 2023, 10:56 AM

#

felt more meaningful that way, also eases the range parsing part, it's a "special" range value

#

I'm not married to it though

scarlet bobcat Dec 12, 2023, 10:56 AM

#

seems like extra typing pain tbh

tidal cloak Dec 12, 2023, 10:57 AM

#

that's fair but I think it's more readable, you want it removed?

scarlet bobcat Dec 12, 2023, 10:57 AM

#

the collision of the two usages of - is the problem?

tidal cloak Dec 12, 2023, 10:57 AM

#

scarlet bobcat the collision of the two usages of `-` is the problem?

yea

scarlet bobcat Dec 12, 2023, 10:58 AM

#

but autocomplete never has a range right?

#

can I also use it in normal tests, e.g. 1--1?

#

or maybe in your case 1 - backtick - 1 backtick?

tidal cloak Dec 12, 2023, 10:59 AM

#

okay, I checked and I was wrong, it doesn't error the second annotation anymore, it must have been a weird bug on my part

#

I don't understand what 1--1 means

scarlet bobcat Dec 12, 2023, 10:59 AM

#

from 1 to -1

#

I'm not advocating for it, just asking whether it naturally arose from your implementation

tidal cloak Dec 12, 2023, 11:00 AM

#

No I only modified the range parsing, you can use -1 in any test but it is translated as the range -1..-1

#

since autocomplete doesn't' need a range

scarlet bobcat Dec 12, 2023, 11:00 AM

#

so it only works in isolation, not in a range?

tidal cloak Dec 12, 2023, 11:00 AM

#

I mean, it is a range, but it only works if you need an index

#

right now if someone passes a range with len != 0, i throw a warning in autocomplete

scarlet bobcat Dec 12, 2023, 11:01 AM

#

I mean just what is valid syntax for ranges

#

1, -1, 1-2 for sure

tidal cloak Dec 12, 2023, 11:01 AM

#

what we had before + -1

scarlet bobcat Dec 12, 2023, 11:02 AM

#

but are -1-2 probably not right?

tidal cloak Dec 12, 2023, 11:02 AM

#

1 is not a range tho

scarlet bobcat Dec 12, 2023, 11:02 AM

#

(range shorthand)

#

anyway, let's get rid of the backticks

tidal cloak Dec 12, 2023, 11:02 AM

#

Okay for getting rid of the backticks

#

should we accept -1 as a position instead of a range then?

#

then -1--1 would be accepted but I don't love it

scarlet bobcat Dec 12, 2023, 11:03 AM

#

I don't mind either way

#

People are probably gonna complain about the confusing range syntax

tidal cloak Dec 12, 2023, 11:03 AM

#

thats fair, that's why I liked -1 as a shorthand, can we have that without the backticks?

scarlet bobcat Dec 12, 2023, 11:03 AM

#

I don't see why not

tidal cloak Dec 12, 2023, 11:04 AM

#

but then, let's keep the rest of the logic, no -1--1, not -1 as a position

scarlet bobcat Dec 12, 2023, 11:04 AM

#

okay

tidal cloak Dec 12, 2023, 11:04 AM

#

sounds great 😄

#

Well it's almost over and done then!

tidal cloak Dec 12, 2023, 11:57 AM

#

Well, ready for review!

tidal cloak Dec 14, 2023, 1:48 PM

#

@scarlet bobcat
I updated my PR, In order to refactor the error handling in the metadata parsing phase I moved a number of things around to simplify the code.
It had the nice properties of being able to error on invalid metadata keys which is really useful instead of just ignoring it.

The parsing of it gets ... bad though

currently it still ignores invalid keys cause these would be parsed as keys:
// See also: https://github.com/mTvare6/hello-world.rs
// Should output Name: Typst. Age: 2..

Plus the parsing is a bit messy not a big big deal but still

 let metadata = |s: &mut Scanner<'a>| -> Option<(String, &'a str)> {
        if !s.eat_if("//") {
            return None;
        }
        s.eat_whitespace();

        let key1 = s.eat_while(|c: char| !c.is_whitespace() && c != ':');
        if key1.is_empty() {
            return None;
        }

        s.eat_whitespace();
        let key2 = s.eat_while(|c: char| !c.is_whitespace() && c != ':');

        let key =
            if key2.is_empty() { key1.to_string() } else { format!("{} {}", key1, key2) };

        s.eat_whitespace();

        if !s.eat_if(':') {
            return None;
        }

        let value = s.eat_until('\n');
        Some((key, value))
    };

I would like to replace this all mess with s.eat_until(':')

a possible solution for this would be to force metadata to start with /// (or even \\\\ or \\! for forward compatibility) instead of //

It would allow to error on invalid metadata keys as well.

It requires changing 19 lines (17 tests)

#

I feel like that's not too big a change (I can do it obviously), Should I go forward with it?

scarlet bobcat Dec 14, 2023, 1:50 PM

#

what does /// give us over //? I don't yet understand.

tidal cloak Dec 14, 2023, 1:51 PM

#

current parsing consumes // then tries to find two words then consumes :
The problem is some comments starts the same way but are not metadata

#

examples: ```typ
// https://github.com/typst/typst/issues/2162
// Test whether relative: "parent" works correctly on conic gradients.
// Make sure they don't work when relative: "self".

tidal cloak Dec 14, 2023, 1:53 PM

#

scarlet bobcat what does `///` give us over `//`? I don't yet understand.

/// would disambiguate it

#

It requires changing 19 lines (17 tests)
That's stupid; we need to change all tests ^^'

#

Or We need to say that // {key} {key}: is reserved and you should use /// which would only require only changing 19 lines, it seems sensible imo

tidal cloak Dec 14, 2023, 2:47 PM

#

@scarlet bobcat

scarlet bobcat Dec 14, 2023, 5:07 PM

#

tidal cloak Or We need to say that `// {key} {key}:` is reserved and you should use `///` wh...

can't we still hardcode which keys can occur?

#

it must start with "Error" or "Autocomplete" or sth like that after all

tidal cloak Dec 14, 2023, 6:12 PM

#

We can but then if the person writes error or Errors it doesn't work but looks like its working which is too easy to miss

We also want to think about it review wise

I guess it's not that much to ask to have /// for either annotations or comments looking like annotations

scarlet bobcat Dec 15, 2023, 11:23 AM

#

@tidal cloak I think we still had a bit of a misunderstanding. I think the setup now is confusing because most of the things that require triple slash disambiguation don't look like an annotation at all to a human. They just happen to have a colon somewhere. I think failing on error: or error : (as opposed to Error:) is reasonable, but failing on correct: is gonna be annoying really quickly.

tidal cloak Dec 15, 2023, 11:27 AM

#

the problem is how to differentiate between error and correct

#

there's no super easy way to check that something is a typo

scarlet bobcat Dec 15, 2023, 11:30 AM

#

to_lowercase is easy and would catch error

tidal cloak Dec 15, 2023, 11:30 AM

#

I could create a score that measures how like one of the annotation a string is and then we'd arbitrarily choose a max score but that seems brittle

I think it's okay to require some comments to have /// It's not that big of a deal imo

scarlet bobcat Dec 15, 2023, 11:30 AM

#

if you really think we need to catch other stuff, there is also hamming distance

tidal cloak Dec 15, 2023, 11:30 AM

#

scarlet bobcat to_lowercase is easy and would catch `error`

that's true

#

I was thinking of something like that

scarlet bobcat Dec 15, 2023, 11:30 AM

#

I think it is much more annoying if it randomly interprets something correct as an annotation than if it silently ignores something wrong as not being an annotation.

#

It's also not that silent because if you don't annotate an error that happens, the test fails

tidal cloak Dec 15, 2023, 11:31 AM

#

That's fair enough! Let's just ignore invalid keys for now!

#

Yes but if you annotate an error that isn't there (bug) and you fail your annotation (silent) you think you fixed a problem, but is that a big deal? not really

#

@scarlet bobcat Done!

#Tests for autocompletion