Closed pke closed 4 years ago
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:
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.
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.
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.
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.
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.
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.
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: https://github.com/microsoft/vscode/issues/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).
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.
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 ;)
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?
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.
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
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.
This is now blocked by https://github.com/microsoft/vscode/issues/91852. The selectionRangeProvider has a bug in 1.42. We must wait until the march release, 1.43.
Ah, do you know when the release will be?
The issue is scheduled for the March 2020 release. They have an iteration plan here https://github.com/microsoft/vscode/issues/92242, which says that the release should be ready in early Apri.
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.
This is now part of 2.0.0
.
@pke, I forgot to ask, do you want to be listed as contributor?
Sure why not.
-----Original Message----- From: "fvclaus" notifications@github.com Sent: 03/05/2020 07:50 To: "fvclaus/vsc-sort-json-array" vsc-sort-json-array@noreply.github.com Cc: "Philipp Kursawe" phil.kursawe@gmail.com; "Mention" mention@noreply.github.com Subject: Re: [fvclaus/vsc-sort-json-array] feat: Detect cursor position inarray to sort it (#2)
@pke, I forgot to ask, do you want to be listed as contributor? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
With this there is no need to select the array before sorting it. Just place the cursor anywhere in the array and sort it.
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.