Rename HighlightLines::highlight() to highlight_line() for clarity

[epage]

With cobalt when I have the following code block

def f(x):
  pass

def g(x):
  pass

it gets highlighted as:

See cobalt-org/cobalt.rs#802

If I switch from regex-fancy to regex-onig, it works.

Unsure if this is a dupe of #287

[keith-hall]

Thanks for reporting. If it were an issue with regex-fancy (or unsupported regex syntax), I'd expect to see some failures for the Python syntax tests, but there are none:
0a74d87#diff-fd8f6befd157766171a7487c79ca80cdfde365fb6900a00c72f3cc89dfcbeaedR1

If I understand correctly, Cobalt is using the assets bundled by default with syntect, right? So it is not like its using a syntax definition syntect doesn't support yet.

I'll try to make some time to investigate further.

[keith-hall]

interestingly, executing the synhtml example with fancy-regex vs onig shows no difference:

cargo run --no-default-features --features default-fancy --example synhtml ./testdata/Packages/Python/syntax_test_python.py > synhtml_fancy.html
cargo run --no-default-features --features default-onig --example synhtml ./testdata/Packages/Python/syntax_test_python.py > synhtml_onig.html
cksum ./synhtml*.html
diff --report-identical-files ./synhtml_fancy.html ./synhtml_onig.html 
Files ./synhtml_fancy.html and ./synhtml_onig.html are identical

3954740958 127590 ./synhtml_fancy.html
3954740958 127590 ./synhtml_onig.html

The same with the test Python file you provided, and it looks correct:

I agree from your scenario that if it works with onig, but not fancy-regex that it seems like a fancy-regex problem, but I'm confused why I'm not seeing the same thing with the synhtml example. The only difference I can see is that Cobalt is using highlighted_html_for_string and synhtml is using highlighted_html_for_file. I guess I need to investigate that next :)

[epage]

If I understand correctly, Cobalt is using the assets bundled by default with syntect, right? So it is not like its using a syntax definition syntect doesn't support yet.

Correct, we are using the default assets. Thanks for looking into this!

[keith-hall]

I have tweaked the parser to debug print the "operations", pointed cobalt to my local syntect changes, built it and served it and saw that the whole code block string is being passed into a syntect method which expects a single line at a time - here is a snippet from the output:

def g():
    pass
       ^ +punctuation.section.function.begin.python
def f():
    pass

def g():
    pass
        ^ pop 2
def f():
    pass

def g():
    pass
             ^ +keyword.control.flow.python

Using Liquid templates instead of Markdown in the post-1.md file works as expected:

{% highlight python %}
def f():
    pass

def g():
    pass
{% endhighlight %}

So I hate to say it, but it looks like a bug in cobalt.rs and the way Markdown code blocks are passed into syntect...

As for why does Onig give different output to fancy-regex - well, I guess fancy-regex just doesn't behave the same way in some situations, or Onig defaults to multiline regex mode and fancy-regex doesn't or something - I haven't looked into this further.

[epage]

Oh, that inconsistent implementation is weird. Sorry for wasting your time on a bug in my code.

[epage]

Hmm, I wonder if something should be changed about the API (on next major bump), like renaming fn highlight to fn highlight_line to clarify this.

If I'm reading zola right, it has the same problem

• Takes the whole text from pulldown: https://github.com/getzola/zola/blob/4bbf7a62a16194e9a5689831cb396f585fb44199/components/rendering/src/markdown.rs#L198
• Passes that, without modification, to highlight: https://github.com/getzola/zola/blob/4bbf7a62a16194e9a5689831cb396f585fb44199/components/rendering/src/markdown/codeblock.rs#L43

[keith-hall]

Hmm, I wonder if something should be changed about the API (on next major bump), like renaming fn highlight to fn highlight_line to clarify this.

I was thinking something similar :) or to add a highlight_line method which would be called from highlight - keeping the signature the same to maybe avoid the need for a breaking change.

[Enselic]

I agree highlight_line would probably have been a better name. But on the other hand, the documentation says

Highlights a line of a file

and the parameter name is line.

I think that is clear enough to justify closing this issue to keep the issue inbox clean and organized :)

[epage]

I disagree. People are more likely to be building off of code samples rather than looking at rusdocs and will miss both what the function documentation says and what the parameter name is. As shown in this thread, at least two users of syntect got this wrong. A more specific function name would completely clear this up.

As said, this doesn't need to be a breaking change. A new function can be introduced and the existing function deprecated.

I feel like this is very similar, in spirit, to the "don't use bools" You can say that the documentation and parameter name are sufficient but over time many people have come to the conclusion to exercise caution in using bools in APIs, including the Rust API guidelines. When a parameter's type and position is does not significantly indicate special usage of a parameter, it should be augmented with other information, whether through types or the function name,

[Enselic]

Ok, sure, let's reopen.