Add support for `def 'my-function'` syntax highlighting and add tests to it

[AucaCoyan]

fixes #156

The only caveat is the --flag after the name will be colored just like the function name

[AucaCoyan]

Hi! Does anyone have some time to review this? 😄 Thanks!

[fdncred]

sorry, i'll try to look at it this week sometime.

[AucaCoyan]

No problem! We all have lots of todos, take your time

[fdncred]

@AucaCoyan I spent some time looking at this today and I think the regex changes are not correct. We should be able to capture all groups and using the beginCaptures to highlight them individually. I don't think the groups are correct either.

We should also be able to do export def --env --wrapped blah [] {}.

[fdncred]

We may need @glcraft to help iron out this regex. This one below is close because it sees --env and --wrapped at the beginning or ending.

This is my regex and capture attempt below that doesn't work either because it's uses range 0,2 and therefore is not picking the correct capture names in all instances.

    "function": {
      "begin": "(export)?(\\s+def)(\\s+--\\w+){0,2}(\\s+[\\w\\-]+|\\s+\"[\\w\\- ]+\")(\\s+--\\w+){0,2}",
      "beginCaptures": {
        "1": { "name": "entity.name.function.nushell" },
        "2": { "name": "entity.name.function.nushell" },
        "3": { "name": "entity.name.type.nushell" },
        "4": { "name": "entity.name.type.nushell" },
        "5": { "name": "entity.name.function.nushell" },
        "6": { "name": "entity.name.type.nushell" },
        "7": { "name": "entity.name.type.nushell" }
      },

[glcraft]

Hi I agree with @fdncred, the regex you provided is incorrect, because it will accepts wrong syntaxes like the following :

export def "my-function (...
export def "my-function' (...
export def my-function" (...
export def my function (...

I know that the textmate grammar is not a linter, but due to the huge number of rules present in this file, we have to be precise in the parsing so the good rules are used in the good place.

Now, my previous regex was indeed incomplete. it miss the single quotes function name syntax (and the back quotes too) and I went for one flag only whereas it can be several before and/or after the function name.

@fdncred attempt for matching several flags is good. It will parse the flags correctly. But I think we should set the number of occurence to 0 or more, in case nu syntax change and add more flags to def/Export def in the future. What do you think ?

I'm going to commit a change to the tmgrammar file.

(oh, and sorry for the late about #156, I saw it but I could not fix it at the time and forget about it later 😅)

[fdncred]

But I think we should set the number of occurence to 0 or more, in case nu syntax change and add more flags to def/Export def in the future. What do you think ?

I'm good with 0 or more.

(oh, and sorry for the late about https://github.com/nushell/vscode-nushell-lang/issues/156, I saw it but I could not fix it at the time and forget about it later 😅)

No worries at all.

Thanks for all your input @glcraft

[glcraft]

Oh, it seems i cannot commit onto this PR branch 🤔

[AucaCoyan]

Oh, it seems i cannot commit onto this PR branch 🤔

Thanks for the response! I added you as a collaborator in my fork, check if you can commit again

[glcraft]

Thanks, but no, I cannot. It says I "don't have permission to push to "AucaCoyan/vscode-nushell-lang" on GitHub".

[AucaCoyan]

mmmmm maybe did you received an email, perhaps?

[glcraft]

Yes you're right. I am able to push now, thanks :+1:

[AucaCoyan]

You are welcome! You are also helping me too :smile:

[fdncred]

Someone just ping me when this is ready to look at again.

[glcraft]

@fdncred I fixed and push the change of the tmgrammar file. Check by your side if it's good :+1:

[fdncred]

@glcraft I think this is much better and we can land it. It doesn't cover use case where --env or --wrapped are after the parameter declaration like this.

export def --env my-function [] --wrapped {}

I need to check with the team if that's even supposed to be valid syntax. I think it's invalid because of this.

[fdncred]

Thanks!