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

Full vscode extension #1405

Merged
merged 2 commits into from
Jun 28, 2023
Merged

Full vscode extension #1405

merged 2 commits into from
Jun 28, 2023

Conversation

szlend
Copy link
Contributor

@szlend szlend commented Jun 26, 2023

Ported from: https://github.com/szlend/vscode-nickel

  • Add language definition
  • Add syntax highlighting
  • Replace npm with yarn (npm2nix doesn't support v3 lock format, seems unmaintained)
  • Fix *.ncl file watcher
  • Fix direnv integration

AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a LICENSE we get a warning.

"name": "nls",
"description": "Nickel Language Server",
"name": "vscode-nickel",
"description": "Nickel Language",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be the title (name) and description in the vscode marketplace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick look at the VSCode market place, many extensions (not necessarily languages) seem to follow the vscode-xxx naming scheme, many language extensions don't, but that name sounds reasonable 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, there's also displayName as name can't contain spaces. Do we want something like "Nickel Language" instead? And add some more details in "description“?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that could be a good idea. But this can be done is a subsequent PR as well, so I'm in for merging this one first, as it's self-contained and in good shape 🙂

args: traceFile ? ["--trace", traceFile.toString()] : [],
transport: TransportKind.stdio,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this as it was trying to pass --stdin to nls.

src = pkgs.lib.cleanSource ./lsp/client-extension;

buildPhase = ''
export HOME="$TMPDIR"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's trying to create .yarn in HOME. There's probably a better way to fix this but this works for now.

@szlend szlend force-pushed the full-vscode-extension branch 3 times, most recently from f75b67d to 606f243 Compare June 26, 2023 21:17
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this @szlend ! It's fine by me, I just don't approve yet so that you can take a look at the review comments and decide what to do with them. When it's done, please ping me or re-request my review and I'll merge it.

flake.nix Show resolved Hide resolved
"name": "nls",
"description": "Nickel Language Server",
"name": "vscode-nickel",
"description": "Nickel Language",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick look at the VSCode market place, many extensions (not necessarily languages) seem to follow the vscode-xxx naming scheme, many language extensions don't, but that name sounds reasonable 👍

lsp/client-extension/src/extension.ts Outdated Show resolved Hide resolved
},
"strings_interpolation": {
"name": "string.interpolated",
"begin": "m%\"$",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note, this won't work with variable length string delimiters: m%%\" some string "%%, but honestly we can add this in a second step. To do that properly we need to share some state between begin, end, and patterns, which would be set by begin (and is the number of % in the opening delimiter).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not too familiar with the tmLanguage spec. I added a fix that should at least handle the start and end of variable length string delimiters.

lsp/client-extension/syntaxes/nickel.tmLanguage.json Outdated Show resolved Hide resolved
lsp/client-extension/syntaxes/nickel.tmLanguage.json Outdated Show resolved Hide resolved
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Before publishing we'll take a look at the name/description first, but otherwise this should be a reasonable first version.

@yannham yannham added this pull request to the merge queue Jun 28, 2023
Merged via the queue into tweag:master with commit a4fad2b Jun 28, 2023
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