FIX: Do not autocomplete categories or emojis in code blocks (PR #8459)

GitHub

You’ve signed the CLA, udan11. Thank you! This pull request is ready for review.

The title of this pull request changed from “Disable autocomplete in code blocks” to "FIX: Do not autocomplete categories or emojis in code blocks

I wonder if this one is any faster?

  { rule: "`[^`]+`", end: "`" },

minor… but why map this every time you run the function, just do it once upfront ?

1 Like

hmmm do we want to do this? I get that having auto complete after “test /` @” can be arguably allowed but honestly I am fine to just ban it in this case.

super nice to have this test and nice specification. I wonder though if we should make it a bit more readable?

test V'X'… use V to denote that auto complete will open and X to denote it will not, then you zoom through the string and create assertions on the fly. Way easier to reason about the implementation.

They are both super-fast, but your version looks more readable.

1 Like

You are right. There is also no point in keeping the array of rules anymore. :slight_smile:

1 Like

The user can still end the code block before proceed with its contents, so I will just get rid of this.

The test was built kind of like that. foo was always outside of a code block and bar was always inside a code block. I replaced it with “0” (not in code block) and “1” (in code block) and am generating the assertions on-the-fly now.

I am not a big fan of generating assertions because then I feel like the test contains too much logic. I do agree though that it is a lot more readable.

From my perspective this PR is good to merge, but I am a bit concerned about an assertion for performance, a single GC can through it off so this is ultra likely to trip. I think we should skip this test for now despite the awesome intent.

Maybe we should start thinking about a “performance” front end test suite longer term that handles this and more.

At the integration level this becomes mega interesting, maybe “typing in the composer does not lead to lag” type of integration smoke test. Probably worth discussing with @ZogStriP and @eviltrout

I tried to take into account the system noise by ensuring that the limit is 10 times of what I am seeing on my laptop. Something else I could do to consolidate this test is to run the code 10 times and take the minimum execution time. I doubt the GC can impact more than a few of these runs.

Measuring the minimum time makes sense because ultimately the execution time is a sum of the “real” execution time and noise. By taking the minimum, we are also taking the minimum noise, because in theory the execution time should stay constant. In other words, time = real + noise, min(time) = min(real + noise), but min(real) ~= real so min(time) = real + min(noise).

Anyway, for now, I will remove this test. I agree that it would be helpful to implement a framework for this kind of test and also to write tests for vital components such as the composer or topic view. Chrome kind of does this by console.warning the user if an event handler takes too much to run.

I definitely think this should be a different class of test like smoke, because I guarantee we will see it fail in places we never expected it to otherwise.

As a future project we could add a “speed regression” suite that lets us know if things get slower.