atom / language-csharp

C# language support for Atom
Other
62 stars 52 forks source link

Incorrect string highlight for parameter attributes #22

Closed Measurity closed 9 years ago

Measurity commented 9 years ago

image

Beginning " is not matched.

winstliu commented 9 years ago

Refs #15.

Measurity commented 9 years ago

Refs #13.

winstliu commented 9 years ago

Inlining text for convenience:

namespace Test
{
    public class Test
    {
        public void Execute([BotDisplay(Description = "Command to execute on the server.")] string command)
        {
            // Wrongly highlighted.
        }
    }
}

Minimal repro case:

public class Test
{
    a([b(c = "Command s")] string q)
    {
        // Wrongly highlighted.
    }
}
pchaigno commented 9 years ago

Looks like the minimal repro case might be even worse:

class Poller {
  A a = b("c");
}

(Reported by @Tanjoodo at github/linguist#2661.)

EDIT: Actually it's a different bug :/ Errors with strings are getting very complicated... EDIT2: After investigating a little further, it is the same bug: This issue is actually happening because of the space in the string (and the fact that it is a string parameter). Minimal repro case:

public class Test
{
    b("Command s"); // bug
}

Without a space:

public class Test
{
    b("Commands"); // no bug
}
winstliu commented 9 years ago

@pchaigno Can you test out #39 and see if that fixes your issue?

Tanjoodo commented 9 years ago

As far as I can tell, in the Atom editor, everything looks right.

I cloned this repo, apm linked got, #39 as a .diff file and git apply 39.diffed. Then I opened this file: https://github.com/Tanjoodo/autoproxy/blob/master/AutoProxy/Poller.cs and it was highlighted correctly. I also tried the minimal repro cases provided by @pchaigno and they both were highlighted correctly.

I wrote in such detail so you could verify that I tested it correctly.

pchaigno commented 9 years ago

It looks good in Lightshow too :-) The b is not highlighted anymore though, is that normal?

Tanjoodo commented 9 years ago

I'm going to guess not because the patch assumes the method regex was overeager.