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

(python) exec and print should be highlighted as built-ins #1468

Closed
Lestoroer opened this issue Mar 8, 2017 · 15 comments · Fixed by #3045
Closed

(python) exec and print should be highlighted as built-ins #1468

Lestoroer opened this issue Mar 8, 2017 · 15 comments · Fixed by #3045
Labels
bug good first issue Should be easier for first time contributors help welcome Could use help from community language

Comments

@Lestoroer
Copy link

If I'm using just print then this is highlight.
But if I'm using print() then this isn't highlight.
<code class="python highlight"></code>

@isagalaev
Copy link
Member

It's by design. print() is a Python 3 thing where print is not a keyword.

@Lestoroer
Copy link
Author

Highlight js uses python 2?

@isagalaev
Copy link
Member

It tries to highlight Python code of both versions, but makes a guess on how to highlight print depending on how it's used.

@Lestoroer Lestoroer reopened this Mar 9, 2017
@Lestoroer
Copy link
Author

Lestoroer commented Mar 9, 2017

Is it possible to make highligthing print()?
I guess this is used very much on sites and users would like to see highligthing this.

@isagalaev
Copy link
Member

That's unlikely. It's not a keyword in Python anymore, and I don't see the point in highlighting it as one. Also, I'd need some data on how many users actually want it highlighted. It's the first time anyone raises this point.

@Lestoroer
Copy link
Author

Lestoroer commented Mar 10, 2017

I see, Thank you for your work!
But please to think about it because sublime, visio code, eclipse, Python IDE highlighting this.

@typemytype
Copy link

typemytype commented Jan 12, 2018

I really would like you to reconsider print("hello world") in a python 3 world. Its indeed not a keyword anymore, but its part of the built ins. I see that build ins are not at all supported see https://github.com/isagalaev/highlight.js/blob/master/src/languages/python.js#L12-L13

see a list of built ins: https://docs.python.org/3/library/functions.html

thanks

@samuelcolvin
Copy link
Contributor

Is there any chance this could be reconsidered?

As an example, github's highlighting does highlight print:

a = 5
print('foobar', a)

@joshgoebel
Copy link
Member

joshgoebel commented Mar 11, 2021

Read everything above and make a case for why it should be reconsidered. It won't be reconsidered just because you asked nicely - we need a good reason. :)

"GitHub does it this way" isn't a case. :)

@typemytype
Copy link

see t-h-e p-y-t-h-o-n b-u-i-l-d-i-n-s l-i-s-t, don't know how much clearer it should be...

dont reconsider if you want to be stubborn and being stuck in whatever reason this is holding you back, asking nicely should always be a +1 above demanding it

@samuelcolvin
Copy link
Contributor

samuelcolvin commented Mar 12, 2021

"GitHub does it this way" isn't a case. :)

This is a self-evidently incorrect statement. Github doing it this way is a case to change highlight.js's behaviour, it may (on its own, in your opinion) not be a conclusive case, but that doesn't mean it's not a case.

Similarly pygments (the preeminent code highlighter in python) highlights print() as does python's own docs. These are also cases that highlight.js should follow suit.

But I agree with @typemytype that the conclusive argument is that print() is a builtin, so it's an error that it's not wrapped in an element with the hljs-built_in class by hightlight.js.

Remember, we're not suggesting that hightlight.js forces people to highlight print(), just that they make it possible.


Overall, as someone who reviews (and often refuses) feature requests & bugs everyday, the case that this should not be changed is pretty thin.

@joshgoebel
Copy link
Member

joshgoebel commented Mar 12, 2021

This is a self-evidently incorrect statement.

Indeed. I will be clearer next time. You understood my meaning though. :)

Overall, as someone who reviews (and often refuses) feature requests & bugs everyday, the case that this should not be changed is pretty thin.

I tend to assume past behavior has reasons (and they made sense to me last time I looked at this) - and our very own library author made this change himself... so... that makes the burden perhaps a bit higher. :-) That plus a bit of "say no by default" mixed in.

conclusive argument is that print() is a builtin, so it's an error that it's not wrapped in an element with the hljs-built_in class by hightlight.js.

This seems quite compelling. Reviewing. At the time these changes were made (excluding exec and print) we did not track built_ins separately (at least not in this grammar)... they were keywords... so Ivan was taking the correct step of not highlighting them as keywords any longer.

But since we do highlight built-ins it does seems these should again be highlighting - now as built-ins.

@joshgoebel joshgoebel reopened this Mar 12, 2021
@joshgoebel joshgoebel changed the title Python highligth syntax print() doesn't work (python) exec and print should be highlighted as built-ins Mar 12, 2021
@joshgoebel joshgoebel added bug good first issue Should be easier for first time contributors help welcome Could use help from community language labels Mar 12, 2021
@samuelcolvin
Copy link
Contributor

Thanks for reconsidering, I agree entirely with "say no by default", but I think making an exception to that rule here is a good decision.

@samuelcolvin
Copy link
Contributor

PR created.

@isagalaev
Copy link
Member

isagalaev commented Mar 13, 2021

I'm still subscribed to this ticket for some reason, so I've read the recent back and forth… I should say @joshgoebel is a saint, were it for me I'd closed and sealed it simply because of sheer amount of entitlement and rudeness in this "discussion". Especially considering that all it should've taken would be pointing out that print is already in built-ins and just doesn't work for some reason. Then it would be a matter of fixing a bug, instead of grand-standing your opinion what someone else's project "should follow".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Should be easier for first time contributors help welcome Could use help from community language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants