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

Highlighting trailing comma assignment #14

Open
3 tasks
shazow opened this issue Mar 6, 2014 · 2 comments
Open
3 tasks

Highlighting trailing comma assignment #14

shazow opened this issue Mar 6, 2014 · 2 comments

Comments

@shazow
Copy link

shazow commented Mar 6, 2014

Hi there,

I find that bugs like

foo = 42,

are fairly common that it would be valuable to highlight the , in that scenario. While this syntax is technically correct in creating a tuple, I find that it's unintentional 80% of the time and drawing more attention to it would be valuable.

Closest solution I've hacked together is:

syn match PythonTrailingCommaWarning    "[=].*\(,\)\s*\(#.*\)*$" display

I'd love for something like this to be included in this syntax file, but there are some improvements that could be done if anyone is more familiar with how Vim syntax files work:

  • Only highlight the ,, not the entire substring starting from = ...
  • Include the scenario where it's return ...,
  • Exclude the scenario where it's foo = "bar, #"

Though I find that even without these improvements, this addition is already useful in catching these annoying bugs. :)

@seanbell
Copy link

I like this idea. There is another instance that you need to exclude:

some_function(
    kwarg1=val1,
    kwarg2=val2,
)

Since I use no spaces for kwargs, you can modify the regex (I added spaces around [=]) to exclude this case:

syn match PythonTrailingCommaWarning    " [=] .*\(,\)\s*\(#.*\)*$" display

I feel like what you are suggesting needs to be done at the level of a linter that has constructed an abstract syntax tree. It may not be doable with regexes.

@shazow
Copy link
Author

shazow commented Mar 18, 2014

Yea, including a mandatory space would help in scenarios where PEP8 is observed.

@wolever and I looked into doing this within a linter but couldn't find a reasonable entry point. Would love a third pair of eyes if anyone has experience with these kinds of things. :)

Personally, I'd be happy with a warning-style highlight of trailing commas just to draw a little extra attention to them.

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

No branches or pull requests

2 participants