Cut inefficient CssClassSet out of content parsing #505
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turns out that `classes.contains` instantiates a Set every time it's called. That's unfortunate. So, remove some of this work by just checking the class string (`className`) directly, in the case where we expect just one class. When we expect two or more classes, we'll want a check that doesn't enforce a particular order, so perhaps we'll use a regular expression for those, or something, in later work. See zulip#497. Fixes-partly: zulip#497
This className string can never be empty, because it's an item from a CssClassSet, and those consist of nonempty strings. (If for example the element's "class" attribute is absent, or empty, or pure whitespace, then the resulting class set still has no empty strings; it'll just be an empty set.)
This is NFC except in the case where className looks like " foo ", with leading and/or trailing whitespace. In that case, the previous code would accept it (if the class name in the middle was one we recognized), and this code will reject as unimplemented. For cases where the previous code would reject because `classes.length` was not 1, this code will reject because className won't be one that codeBlockSpanTypeFromClassName recognizes.
This is more efficient but should otherwise behave exactly the same, except if the className has extraneous whitespace: the previous code would accept that, and this code will reject as unimplemented. To help verify that we've gotten this change right, add some more tests, so that we cover all the different alternatives in this regexp.
We never look at the captured group, so it's more efficient to not ask the regexp engine to capture it in the first place.
|
Thanks, LGTM! Merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #497
The first two commits here are by @chrisbobbe, taken from #503. The later commits go in the same direction for checking a class list is empty, and then handle the remaining couple of places that are a bit trickier, applying regexps to
classNameto cover the different possibilities.