search

elves / elvish

Powerful scripting language & versatile interactive shell
https://elv.sh/
BSD 2-Clause "Simplified" License
5.53k stars 297 forks source link

feat(snippets): add basic snippets #1699

Closed EmilyGraceSeville7cf closed 1 year ago

EmilyGraceSeville7cf commented 1 year ago

This naming convention for snippets has been used. The goal of this naming convention to make snippet names predictable and easily memorizable.

EmilyGraceSeville7cf commented 1 year ago

@xiaq, there are failing tests, I have no idea why (I've checked CI failing details).

krader1961 commented 1 year ago

@xiaq, there are failing tests, I have no idea why (I've checked CI failing details).

It looks like the CI failure is due to a change by @xiaq to my implementation of the "peach &num-workers" feature. That change has introduced non-deterministic behavior of the peach related unit tests. My implementation of those unit tests was deliberately very conservative to avoid this type of non-deterministic failure. In other words, the CI test failure has nothing to do with your change. The failure is because a new unit test, introduced prior to your change, is inherently non-deterministic.

krader1961 commented 1 year ago

@EmilySeville7cfg, you wrote

Nushell naming convention for snippets has been used.

I don't use VScode or Nushell but have a passing familiarity with both. It is unclear from your commit comment is why this change is being made. It may help to expand on the "Nushell naming convention for snippets has been used." rationale in the commit comment.

EmilyGraceSeville7cf commented 1 year ago

@krader1961, the point of my comment is to mention the fact that the way snippets are named is the same as for Nushell. The goal of this naming convention to make snippet names predictable and easily memorizable. As I wrote this convention for Nushell already, I decided just to refer it here to make you know how snippets are named and why. ;) This convention can be used not just for Nushell, for many ones. I just try to keep things standardized across several shell extensions for VS Code as I use several shells.

krader1961 commented 1 year ago

This convention can be used not just for Nushell, for many ones. I just try to keep things standardized across several shell extensions for VS Code as I use several shells.

Okay, but that rationale should be included in the commit comment. My point is that someone like myself, who has some idea what both VScode and Nushell are, without having used them, won't be able to immediately understand the point of this change. Let alone someone who has no familiarity with either technology. Reducing the ambiguity of the commit comment is a net positive.

xiaq commented 1 year ago

There are some things I would change, but I'll merge this and apply those changes as a followup commit.

xiaq commented 1 year ago

@krader1961, the point of my comment is to mention the fact that the way snippets are named is the same as for Nushell. The goal of this naming convention to make snippet names predictable and easily memorizable. As I wrote this convention for Nushell already, I decided just to refer it here to make you know how snippets are named and why. ;) This convention can be used not just for Nushell, for many ones. I just try to keep things standardized across several shell extensions for VS Code as I use several shells.

I think for most users the only thing that matters for these snippets is the prefix, so I wouldn't spend much effort trying to keep the descriptions standardized. Nu also has a lot more different types of syntax than Elvish - for Elvish the snippets are all just commands - so there's less need to adopt a rigorous naming system. As long as future contributors to this snippet file keep things consistent it's fine.