kdl-org / kdl

the kdl document language specifications
https://kdl.dev
Other
1.07k stars 61 forks source link

Allow spaces in type annotations for vertical alignment since () already serve as delimiters #367

Closed eugenesvk closed 6 months ago

eugenesvk commented 6 months ago

Example: let's say you have a keybinding config for a modal editor where you want to pass a mode for a keybind in types. The best way to allow easy visual differentiation of what mode a keybind belongs to would be to have the same mode always positioned in the same column like so:

//(mode)Shortcut "command"
(niv)Escape "switch_to_normal" // works in Normal/Insert/Visual modes
( i )Ctrl+I "switch_to_normal" // obvious that only one mode matches
// and that mode is Insert since 'i' is in the same spot
// and eyeballing for a diff is easier

Compared to this non-aligned view where also node names and closing ) interfere with visual comparison

(niv)Escape "switch_to_normal"
(i)Ctrl+I   "switch_to_normal"

But currently spaces are not allowed in types even though they aren't needed: you already have () to signal limits to the type (banning spaces in regular bare identifiers makes sense since spaces are used to separate different values)

Currently there is a workaround: use quotes, but for short types that would be using more symbols for delimitation than for content: ("")

("niv")Escape "switch_to_normal"
(" i ")Ctrl+I "switch_to_normal"

P.S. Or maybe just using the good old _ instead of is just as fine

(niv)Escape "switch_to_normal"
(n_v)Ctrl+I "switch_to_normal"
IceDragon200 commented 6 months ago

Spaces in type annotations are allowed in kdl v2.

So keep an eye on the PR #286

eugenesvk commented 6 months ago

That's better, though as far as I understood it would not allow whitespace within type values (since unquoted strings do not allow whitespace), so this would still be illegal

(niv)Escape "normal_mode"
(n v)Ctrl+I "normal_mode"
IceDragon200 commented 6 months ago

Type annotations are not meant to be used in that manner, it denotes one value which "suggests" what the type should be.

Rather I believe if you flip the document around you achieve something slightly more tolerable

(normal_mode)Escape niv
(normal_mode)Ctrl+I n v

Where the type annotation now denotes the mode and the arguments denote the key bindings This has the benefit of allowing you to annotate the keys themselves now:

(normal_mode)Escape (upper-only)niv
(normal_mode)Ctrl+I n (icase)v

You could also use sub nodes instead to denote the modes to remove the visual clutter of the annotations

normal_mode {
  Escape niv
  Ctrl+I n v
}
eugenesvk commented 6 months ago

Type annotations are not meant to be used in that manner, it denotes one value which "suggests" what the type should be.

A combination of modes is a type for a keybind! It's just that given the already existing delimiters () I thought we could get more flexibility by basically allowing (almost all of ) quoted strings without quotes (excluding ())!

Rather I believe if you flip the document around you achieve something slightly more tolerable

(normal_mode)Escape niv
(normal_mode)Ctrl+I n v

Where the type annotation now denotes the mode

That's a command, not the mode, apologies for a confusing example, normal_mode here is switch_to_normal_mode. Also, there can be a sequence of commands, not just one (like switch_to_normal_mode move_word_left switch_to_previous_mode, so they wouldn't fit in a single type (besides, they aren't a type, so that's also a category fit issue). Besides the command names can be rather long, so that would break the nice vertical alignment

You could also use sub nodes instead to denote the modes to remove the visual clutter of the annotations

normal_mode {
  Escape niv
  Ctrl+I n v
}

The plan was to have user-definable categories with the same mechanism of (types) to set modes per category, so (this particular example fails since mode switching would not have a common mode, but just as a bad illustration)

(n)"my mode switching commands" {
  // by default, only work in normal mode
  I "switch_to_insert_mode"
  V "switch_to_visual_mode"
  // but can also be overriden for any given keybind
  (i)Ctrl+I "switch_to_normal_mode"
}
IceDragon200 commented 6 months ago

Hmm, I could see this getting out of hand really quick if we allowed multiple values in annotations, I'd like to get the feedback from the rest of the team on this one

eugenesvk commented 6 months ago

The idea was that it would still be parsed as a single value, though, so for ( n v ) Ctrl+X "cut_selection" the type value that the parsed app gets is

But it might indeed be too complicated to introduce another almost-quoted string-without-quotes-and-parentheses

IceDragon200 commented 6 months ago

Yeah at that point you just quote the whole thing, a little annoying but it achieves what you'd need

// Since bare identifiers are allowed in kdl2 you can skip the quotes on the arguments now as long as it's an allowed identifier
("n v")Ctrl+X cut_selection
zkat commented 6 months ago

This is already integrated as part of kdl v2 and there was already a discussion about multiple values in them and we decided against it, so in closing this as a duplicate of that.

tabatkins commented 6 months ago

it would not allow whitespace within type values

Right, ident strings are terminated by whitespace, so you have to use one of the other string types if you want to write a type annotation with whitespace in it.

eugenesvk commented 6 months ago

was already a discussion about multiple values in them

this is about a single value that allows using space without "" since () can perform the same function of separating different elements instead of

IceDragon200 commented 6 months ago

The thing is the annotation spec is basically this:

annotation = "(" (ident | string) ")"

And idents don't allow spaces hence why it's not supported at all, you need a string to have spaces in it

eugenesvk commented 6 months ago

Ok, so this could become annotation = "(" (ident | string | unquoted-string-with-unicode-spaces-because-you-don't-need-spaces-to-terminate) ")"

tabatkins commented 6 months ago

In v2 the grammar is '(' string ')' (plus some whitespace, omitted for clarity) and idents are now just a type of string.

As was already stated, allowing multiple annotations on a value is intentionally not supported, and allowing a special-case syntax for writing an, I guess, "multi-part ident string" that only works in one spot isn't worthwhile. Better to have a simple, consistent grammar everywhere.