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

feat: Detect cursor position in array to sort it #2

Conversation

pke
Copy link

@pke pke commented Oct 19, 2019

With this there is no need to select the array before sorting it.
Just place the cursor anywhere in the array and sort it.
IpvcsBDKOM

If this works good enough we might add AST parsing for the selection the
same way. That would ensure that even partially selected arrays get properly
detected.
Or we can ditch the selection based detection all together.

Then, since we have a proper AST of the array now, we could (instead of
parsing it using JSON.parse) sort the AST elements and maybe then
transform the AST back to source code to not change the code
formatting like it is happening now.

If this works good enough we might add AST parsing for the selection the
same way. That would ensure that even partially selected arrays get properly
detected.
Or we can ditch the selection based detection all together.

Then, since we have a proper AST of the array now, we could (instead of
parsing it using JSON.parse) sort the AST elements and maybe then
transform the  AST back to source code to not change the code
formatting like it is happening now.
@pke pke mentioned this pull request Oct 20, 2019
@fvclaus
Copy link
Owner

fvclaus commented Oct 20, 2019

Thanks for contributing. I really like the idea and the usability and prospect of not fucking up the formatting, but during the code review I stumbled upon a few things on the line that invokes the acon parse method. I initially typed this as comment, but I think this discussion is better had here:

Error handling

This is lacking error handling. parse will throw in case of an syntax error. I tried this and the exception message actually appears as error message in vscode, even though the extension code does not handle this. I am not sure why this happens, because the code is running in a promise. I thought this will be swallowed silently and it may not be a good idea to rely on the current behavior. Anyway, the error message is my example is Unexpected token (1:3). I guess a Could not sort array, because there is a syntax error: Unexpected token (1:3) will be sufficient.

Ecmascript version and other options

parse uses by default Ecmascript version 10. I don't really keep track of the versions, but that seems to be the most current one now. But in a year, this would need manual upgrading. Furthermore they only supported stage 4 features and require configuration for a bunch of other options such as allowAwaitOutsideFunction, allowImportExportEverywhere and allowHashBang. Every user that has a babel configuration with different options would be prevented from using this extension.

Other programming languages

This also doesn't work for typescript files. Typescript probably requires usage of the typescript module similar to my code that does the custom sorting. It also won't work with other languages that just happen to have the same syntax for specifying JSON arrays like Python. It doesn't even need to be a programming language. Sorting could be required in a README or a log file.

I am not sure if these are valid use-cases, but technically it is possible and I think it would be mistake to prevent users from doing this. Maybe it would be a good idea to do both. Try to parse the file and try to determine the enclosing array and if that doesn't work use the current selection.

@pke
Copy link
Author

pke commented Oct 20, 2019

Right, I totally forgot about the error handling. Silly me. I'll add it.
VSCode accounts for those lazy extension developers and catches unhandled exceptions, so faulty extensions do not bring down the whole VS.

I can also try to use the typescript compiler, which comes with each VS installation to be up to date. The TSC can compile ECMA script too and I think JSX/TSX too.

Of course, as I proposed, the 2 methods of detecting the sortable array should exist next to each other.
Getting the formatting done in a non JS/JSON file is more difficult though.

@pke
Copy link
Author

pke commented Oct 21, 2019

I've checked the arcon docs again and you are right, they are not up to date with current proposals. I guess I'll select the typescript compiler coming with JS for all json/[java|type]script documents and for all other we rely on the selection algo as a fallback.

@fvclaus
Copy link
Owner

fvclaus commented Oct 21, 2019

I thought a little bit more about this issue. I think the command, in its current form, is not as widely applicable as I think, because it uses JSON.parse to parse the array. This requires strict compliance with the JSON syntax. So {'a': 1} or {a: 1} will fail to parse and because single-quote seems to be the de-facto standard for js, most developers will not be able to use this command in JS/TS files. Solving this is not a concern for this ticket, but I think using the typescript compiler is a good solution here, if it supports JSON/JS/JSX/TSX and TS files. As mentioned above, I think the most relevant will be JSON anyway. If it doesn't work better than acorn, using acorn will still significantly improve the user experience.

Adding support to files that are not parsable by typescript or acorn could be done using an algorithm that works similiar to the smartSelect.expand command. A mini-parser so to speak that will try to find the enclosing array. I tried to work out something in pseudo-code, thinking that this should be "not so complicated", because the JSON syntax is quite restrictive. Turns out that this is still really complicated. I then looked at the source code for the smartSelect.expand command. It uses tokenized lines to determine word and bracket boundaries. It actually exposes a command that "could" be used: vscode.executeSelectionRangeProvider.

It works something like this: await vscode.commands.executeCommand('vscode.executeSelectionRangeProvider', document.uri, [selection.active]). If the cursor is on the word "value" in this file

[
    "foo",
    {
        "value": "bar"
    }
]

it will return the following result:

"[
  {
    "range": [
      {
        "line": 3,
        "character": 9
      },
      {
        "line": 3,
        "character": 14
      }
    ],
    "parent": {
      "range": [
        {
          "line": 3,
          "character": 8
        },
        {
          "line": 3,
          "character": 15
        }
      ],
      "parent": {
        "range": [
          {
            "line": 3,
            "character": 8
          },
          {
            "line": 3,
            "character": 22
          }
        ],
        "parent": {
          "range": [
            {
              "line": 3,
              "character": 0
            },
            {
              "line": 3,
              "character": 22
            }
          ],
          "parent": {
            "range": [
              {
                "line": 2,
                "character": 5
              },
              {
                "line": 4,
                "character": 4
              }
            ],
            "parent": {
              "range": [
                {
                  "line": 2,
                  "character": 4
                },
                {
                  "line": 4,
                  "character": 5
                }
              ],
              "parent": {
                "range": [
                  {
                    "line": 2,
                    "character": 0
                  },
                  {
                    "line": 4,
                    "character": 5
                  }
                ],
                "parent": {
                  "range": [
                    {
                      "line": 0,
                      "character": 1
                    },
                    {
                      "line": 5,
                      "character": 0
                    }
                  ],
                  "parent": {
                    "range": [
                      {
                        "line": 0,
                        "character": 0
                      },
                      {
                        "line": 5,
                        "character": 1
                      }
                    ]
                  }
                }
              }
            }
          }
        }
      }
    }
  }
]"

Not sure if commands that are prefix with vscode are considered internal only or not. I think this is too much for this ticket anyway and should be implemented in another ticket since it will probably affect few users.

I think we can wrap up this ticket with the extended error handling and usage of typescript, acorn and fallback to the current selection. If there is no selection, we should display a useful error message.

@fvclaus
Copy link
Owner

fvclaus commented Oct 22, 2019

I created a new ticket #7 to enable sorting on JS arrays that are not valid JSON arrays. I was trying to find out whether the vscode.executeSelectionRangeProvider is public API. https://code.visualstudio.com/api/references/commands lists commands prefixed with vscode that follow a similiar pattern, executeXYZProvider. I also found the vscode.executeFormatRangeProvide which does return a TextEdit[] datastructure given a Range that formats a range in the document in accordance with the Format document command.

const formattingTextEdits: vscode.TextEdit[] = await vscode.commands.executeCommand('vscode.executeFormatRangeProvider', document.uri, new vscode.Range(selection.start, selection.end), {
                    tabSize: editor.options.tabSize,
                    insertSpaces: editor.options.insertSpaces
                } as vscode.FormattingOptions) as vscode.TextEdit[]
const workspaceEdit = new vscode.WorkspaceEdit()
workspaceEdit.set(document.uri, formattingTextEdits)
await workspace.applyEdit(workspaceEdit);

Maybe this is useful later.

I did find the executeSelectionRangeProvider command here: microsoft/vscode#75746. This and they are both defined in the same file: https://github.com/microsoft/vscode/blob/0fc92f222f9b7007240583c090d5c2c748d91938/src/vs/workbench/api/common/extHostApiCommands.ts#L216

So I think this may be the best solution for parsing an array (if it actually works, which only testing can show).

@pke
Copy link
Author

pke commented Oct 22, 2019

Ok, lets try that. I tried to find out how the bracket highlights work and it seems they use some kind of TextMate matcher (regex?). But this functionality is not exposed but maybe its worth a look how they detect array bounds. I also briefly thought I had figured out a reg-ex to find the outer array, but I discarded it in favour of the acorn solution which looked more solid at the time.

@fvclaus
Copy link
Owner

fvclaus commented Oct 24, 2019

That sounds interesting. Do you have a starting point in the vscode codebase for the bracket highlight functionality?

For your regex solution, did you consider the following scenario:

[
  {
    "foo": "[][][\"",
    "bar": "a[bx]d",
    "array": [1, 2, 3],
    "baz": '][]]][][]]]]'
  }
]

Assuming the cursor is somewhere in the bar before b or x. Closing and opening brackets can also occur in strings. Sticking strictly to JSON, that means looking for a " delimiter. But you also need to account for escaped quotes, \". If one would want to move away from JSON.parse to eval or similiar to support JS arrays that are not JSON arrays, then the parsing gets even more complicated, because now strings are either delimited by ', " or `. I stumbled over this in the attempt to formulate the JSON mini-parser. I think there is no sane implementation that does not use proper tokenization. But then again, I may be wrong ;)

@pke
Copy link
Author

pke commented Nov 10, 2019

Sorry for the delay. Kind of busy the last 2 weeks.
So the highlighting is done using TextMate regex they are loading. But I haven't looked any closer to it.

How do we want us to proceed here?

@fvclaus
Copy link
Owner

fvclaus commented Nov 29, 2019

Sorry for the delay. Just so I understand properly: Highlight is when you put the cursor into an array and it shows the array boundaries? And this is implemented using a regex? What is a TextMate regex?

If this works with a regex, I guess that would leave us with two options, the executeSelectionRangeProvider or the regex. I have no strong opinion on either. I suspect the regex might be easier to implement and read, because the command requires us to manually find the boundaries. It also doesn't tie the code to the vscode API and requires us to raise the minimum version for this extension (executeSelectionRangeProvider was introduced in June this year, I think). So I would try the regex first.

When you post the regex here, I would be happy to test it.

@pke
Copy link
Author

pke commented Nov 30, 2019

TextMate is a famous text editor on the Mac platform.
VSCode seems to have taken its syntax files and use them.
You can find them here:
c:\Program Files (x86)\Microsoft VS Code\resources\app\extensions\javascript\syntaxes\

I am not sure, if that really helps us though. But maybe you find the right array regex in there somewhere in JavaScript.tmLanguage.json

fvclaus added a commit that referenced this pull request Feb 15, 2020
fvclaus added a commit that referenced this pull request Feb 23, 2020
fvclaus added a commit that referenced this pull request Feb 23, 2020
fvclaus added a commit that referenced this pull request Feb 23, 2020
fvclaus added a commit that referenced this pull request Feb 23, 2020
@fvclaus
Copy link
Owner

fvclaus commented Feb 23, 2020

I was in the mood to work on this a little. I used the executeSelectionRangeProvider which is really easy to integrate and works on everything that I have tried so far. I will do some more manual testing, go over the existing tests and do some cleanup and then get ready to release a new version.

@fvclaus
Copy link
Owner

fvclaus commented Mar 11, 2020

This is now blocked by microsoft/vscode#91852. The selectionRangeProvider has a bug in 1.42. We must wait until the march release, 1.43.

@pke
Copy link
Author

pke commented Mar 12, 2020

Ah, do you know when the release will be?

@fvclaus
Copy link
Owner

fvclaus commented Mar 12, 2020

The issue is scheduled for the March 2020 release. They have an iteration plan here microsoft/vscode#92242, which says that the release should be ready in early Apri.

@fvclaus
Copy link
Owner

fvclaus commented Mar 22, 2020

This will not be part of 1.43, but 1.44. I just tested the 1.44-insiders and the selectionRangeProvider seems to work fine again.

@fvclaus
Copy link
Owner

fvclaus commented May 1, 2020

This is now part of 2.0.0.

@fvclaus fvclaus closed this May 1, 2020
@fvclaus
Copy link
Owner

fvclaus commented May 3, 2020

@pke, I forgot to ask, do you want to be listed as contributor?

@pke
Copy link
Author

pke commented May 3, 2020 via email

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

Successfully merging this pull request may close these issues.

2 participants