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) Decorator syntax breaks comment formatting #2804

Closed
textbook opened this issue Oct 30, 2020 · 7 comments · Fixed by #2811
Closed

(python) Decorator syntax breaks comment formatting #2804

textbook opened this issue Oct 30, 2020 · 7 comments · Fixed by #2811
Labels
bug good first issue help welcome language

Comments

@textbook
Copy link
Contributor

@textbook textbook commented Oct 30, 2020

Describe the issue
If you have a comment on the same line as a decorator, the whole line gets the hljs-meta class; the comment section (# onwards, see examples below) should be hljs-comment instead.

Which language seems to have the issue?
python

Are you using highlight or highlightAuto?

Based on https://meta.stackoverflow.com/a/401590/3001761 I believe it's using highlight, as the Python tag enables the language selection.

Sample Code to Reproduce

Spotted on: https://stackoverflow.com/q/38031066/3001761 (I've added an extra comment on the line below to highlight the expected formatting more clearly):

Screenshot 2020-10-30 at 11 30 06

Code:

@pytest.mark.asyncio  # note use of pytest-asyncio marker
async def test_async_for():  # but this comment works 🤔
    async for _ in TestImplementation():
        pass

JSFiddle: https://jsfiddle.net/zfe9r6w5/1/

Expected behavior
Comment is greyed out on both lines; GitHub gets this right, as shown in the code sample above.

Additional context
N/A

@textbook textbook added bug help welcome language labels Oct 30, 2020
@textbook
Copy link
Contributor Author

@textbook textbook commented Oct 30, 2020

Looking at the current implementation, it's assumed that any line starting @ will be meta throughout:

{
className: 'meta',
begin: /^[\t ]*@/, end: /$/
},

@joshgoebel joshgoebel added the good first issue label Oct 30, 2020
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Oct 30, 2020

Yep, should be a pretty easy fix (allow comments inside that rule)... want to try a PR?

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Oct 30, 2020

Actually what we really may want is the ending to just end if it sees a comment upcoming:

 { 
   className: 'meta', 
   begin: /^[\t ]*@/, end: /$|(?=#)/ 
 }, 

Or some such... Though a proper regex to match just the decorator would be good if that's a known thing.

@textbook
Copy link
Contributor Author

@textbook textbook commented Oct 30, 2020

a proper regex to match just the decorator

Decorators can be complicated, it's basically @ followed by any expression! You can have decorator factory functions with parameters, for example:

@surround_with("#", repeat=3)
              # ^ not a comment!
def text():
    return "hi!"

text()  # => ###hi!###

I'll have a play with it this weekend and see if I can get a PR together.

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Oct 31, 2020

Can it be like:

@1 + 2 * 6

Then? Or it's gotta be a name or function type expression?

@textbook
Copy link
Contributor Author

@textbook textbook commented Oct 31, 2020

It used to need to be "a dotted name, optionally followed by a single call" per the grammar:

decorator: '@' dotted_name [ '(' [arglist] ')' ] NEWLINE
decorators: decorator+

From Python 3.9, though, as PEP 614 relaxed the rules, it can be any expression per the new grammar:

decorators: ('@' named_expression NEWLINE )+ 

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Oct 31, 2020

If you have a grasp on this I'll just wait to see the PR and markup tests (of which I hope there will be quite a few variants)... but if it truly can be any old expression I'm worried it could be a bit complex... although perhaps all we really need is a simple mode that allows for strings, comments, and then this simply solves itself.

Although I'm curious about the meta styling fighting with the other styles, but we can talk about that once we have a PR I suppose. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue help welcome language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants