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.

Sign up for GitHub

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

Jump to bottom

Empty css spans #276

Merged
merged 3 commits into from
Jan 5, 2020
Merged

Empty css spans #276

merged 3 commits into from
Jan 5, 2020

Conversation

uwearzt
Copy link
Contributor

@uwearzt uwearzt commented Dec 26, 2019

fixes #273

@trishume
Copy link
Owner

I'm sorry about this, but I don't think I'm willing to accept this PR without it using a different solution. This PR adds a dependency on regex when there are already probably too many dependencies and a different regex engine (onig). It's also a pretty inefficient way to do it, not that this code is likely to be too performance sensitive. It's important to me that syntect be as lightweight and fast as I can make it, and in this case adding an extra regex dependency also burdens everyone, even those who don't use this feature.

A better way to do it might be to track the current length of s in tokens_to_classed_spans in a local variable before opening a span, and another flag variable for if any text was added. Then at the code that closes a span, check if any text was added to the span and if not then instead of closing, truncate the string to the stored length before the previous span was opened. If you want to get really fancy you could even have it detect multiple nested spans with no text in them and remove all the levels, which might require a stack, I don't think this is worth it though.

@uwearzt
Copy link
Contributor Author

uwearzt commented Jan 2, 2020

@trishume You're right, regex might be a huge overkill for that purpose... I have reimplemented the feature according to your suggestion. Please have a look.

Thanks in advance
Uwe

@trishume
Undo updates to Cargo.toml
d133329 
They aren't necessary and they increase the minimum required versions
even though syntect doesn't actually need versions past those.

The versions in Cargo.toml are semver minimum required versions so don't
need to be updated except when using new features and for major
releases. Although it's possible we accidentally started using some
features of new versions and they do need updating, let me know if so.
@trishume
Revert breaking API change
b851564 
This changed a public API in a breaking way which would require a major
version release. It's also not necessary and the current API is in
idiomatic Rust style as far as I know.

If the existing API makes it harder to figure out borrow checker issues
you may be able to solve your problem by changing things around so you
can move the HTML generator, or use mem::swap to swap in an empty one.
trishume
trishume approved these changes Jan 4, 2020
View reviewed changes
Copy link
Owner

@trishume trishume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much!

I committed a couple small cleanups of the diff unrelated to your main feature. You can check the commits and messages to see them.

@trishume trishume merged commit e9b942d into trishume:master Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trishume trishume trishume approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ClassedHTMLGenerator generates Empty <span></span> tags.
2 participants
@uwearzt @trishume