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.

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

Remove sprintf format parsing for Rust lexer #1400

Merged
merged 1 commit into from Jan 22, 2020

Conversation

KamilaBorowska
Copy link
Contributor

Rust doesn't use sprintf parsing for its string formatting, using Python-style formatting instead.

For instance, print!("test %?", a.b) would be written as print!("test {:?}", a.b) in Rust today. The old syntax was removed before 1.0, as such there is no real reason to support it.

Rust doesn't use sprintf parsing for its string formatting, using
Python-style formatting instead.

For instance, `print!("test %?", a.b)` would be written as
`print!("test {:?}", a.b)` in Rust today. The old syntax was
removed before 1.0, as such there is no real reason to support
it.
@pyrmont pyrmont self-assigned this Jan 20, 2020
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jan 20, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Jan 20, 2020

I need to have a think but I'm a little hesitant. One of the primary uses of Rouge is as a syntax highlighter for Jekyll-based blogs. It'd suck if you had some pre-1.0 code samples on your blog and they were highlighting fine and then suddenly they started having errors.

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Jan 20, 2020

In this case, %-strings will be formatted as normal strings, which doesn't seem too bad. Nothing in the language considers %-strings to be special (although this may not be the case for external libraries, that said from popular libraries, I think it's pretty much just chrono::format::strftime, and this is time formatting, which is going to be wrong with this rule anyway, as it's designed for pre-Rust-1.0-printf-like-interfaces specifically).

That said, I suppose the Rust lexer also supports some pre-1.0 keywords, which are much bigger of a deal (copy, do, fail, log, priv, pure). do and priv are reserved, but the others are valid identifiers in Rust programs. Similarly, there are some pre-1.0 types.

In fact, log somewhat conflicts with a popular Rust library called log, so it may make sense to remove that one. Or I suppose avoid highlighting log as a keyword if it's followed by }, ,, ;, :: or ! (as this pretty much indicates not using old log keyword). Example of incorrect formatting of log: https://gitlab.com/pastebinrun/pastebinrun/blob/master/src/models/paste.rs.

Just to be clear, I'm aware that assert and drop also are no longer keywords, but they exist in some way in a language (assert! is a macro, and drop is a function).

I suppose not merging this is fine, but at the same time, merging this shouldn't affect the old code samples particularly much (sure, you lose highlighting on %, but it's not a big deal, I feel like).

@pyrmont
Copy link
Contributor

pyrmont commented Jan 22, 2020

@xfix Thanks for the explanation. I agree with you that the fact it'll just be highlighted as part of the regular string limits the adverse impact.

As for the other points, I think those are worthwhile fixes to consider but in a different issue/PR. I'll accept this and merge it in.

@pyrmont pyrmont merged commit db4ddab into rouge-ruby:master Jan 22, 2020
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants