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

Scope subcommand followed by other tokens #23

Merged
merged 44 commits into from
Jul 31, 2023

Conversation

victor-gp
Copy link
Owner

@victor-gp victor-gp commented Jul 18, 2023

Resolves #3

Limitations

  • We cannot scope the sub-tokens following subcommands
    • e.g. "PORT" in adb's tcip PORT
    • because first we need to scope Usage lines across the board
      • we cannot target subcommands only, without impacting usage lines
      • so it would cause a lot of inconsistencies on that front
    • but this is a feature I'm willing to add, it makes sense because
      • usage lines are important, we should highlight them
      • they seem easy enough to target?
    • => Scope usage lines #24
  • Quite a few Usage lines have already become scoped with these changes
    • but they look consistent enough within the same help message
    • and it's a change we plan to propagate everywhere anyways

Pending

  • scope a stray usage line in chown
  • scope a stray usage line in npm

Not required

  • minor issues to be fixed at a later date in jq, dd
  • peccata minuta we can live with in dpkg, ripgrep

Examples

adb:

adb-demo-1

adb-demo-2

adb-demo-3

usage lines:

bat-example

npm-example

random keyword=ARG lines:

dd-example

Terms as in "defined terms".

Mostly for subcommands followed by arguments or other subcommands. Which
are defined terms themselves, but still not handled.

e.g.: "reconnect device         kick connection from device side to ..."

In terms of regression delta, census:

- adb: ok, it motivated this change
- chown: not ideal, examples with chown itself are scoped. but acceptable.
- npm: idem.
- dpkg: not cool, I should limit term sequences to 3-4 terms

Some unhandled cases in adb, chown and npm,

- reveal the single_2_space variable does't work
- show a need to treat definitions that do \n after the terms
I was debugging a problem that didn't exist 😱
It's like it doesn't properly use all the {{long_space}} cases (newline
and single_2_space)
But it's kinda weird because in

 forward --remove LOCAL

it scopes the LOCAL but skips the --remove.

I guess this skip happens in the option-def+space context? Anyways,
worth running the syntest debugger on.

But not today!
For ease of use. Other X is less wrong but a little more unwieldy to
write and reason about.
To reflect the new change in KEYWORD_SCOPES' name
But I'm considering leaving the later unscoped, or any other 2nd term
after a keyword term.

Cause it causes regressions in other, more common commands, where it
scopes terms in examples. (As in every other delta in this commit)
And it introduces a hell of a regression in the Usage section of a few
other commands.

This is breaking for me, we'll probably do the 1st keyword only.
But this introduces quite a bit of regression. See syntax_test_cp
To avoid inconsistent scoping in the usage/examples section of a bunch
of help messages.

But keep these scope assignations as comments, for future reference.
Only the "pair" bit.

This is all so overfitted, to suit adb's help message only, that it's
making me feel icky.

But, on the other hand, it's also a good census of all kinds of patterns
that we don't currently have a proper way to handle.

And if it doesn't add too many unnecessary scopes for other help
messages, I don't care much, I guess?
We're handling subcommand sub-tokens in an explicit manner, without
abstraction / generalization. At least for the time being.
Now we're scoping command keywords in usage lines so as to make them
coherent. Otherwise, some help messages show usage lines with and
without the command colorized.
I hoped to catch the tricky chown case with this, but I think I know why
it's failing: we do not `set: then-pop" in keyword-seq-def matches !!

As a side effect, we scoped "operand=VALUE" operands in dd, which is
nice.

As another side effect, we scoped "or:" usage lines in vim, which is bad
but easy to fix (next commit).
Cause it's an alternative connector in Usage lines
I hoped to catch the chown corner case with this but no such luck.

Instead, we caught a stray adb bug that I had forgotten all about (but
my syntax tests didn't :D ).
To harmonize with the following line, where "bat" is scoped.

This is very hacky but highlighting bat's help message properly is a big
priority for this syntax, as bat is our only playground.

Also renames the test file, cause it was incorrectly versioned 23.0.0
This change also has lots of side-effects.

ghc, ghci and tealdeer are ok

It leaves the jq in the "usage: jq ..." line not scoped, but it's
acceptable to me. Not like bat's case.
Cause keyword=VALUE looks pretty ugly if only the former is colorized.
I moved scripts/env.sh to ./utils in 57748ff and renamed the regression
diff functions from gdiff* to reg* in f8bf10d
Because it was matching the 2-space with the first lookahead, without
consuming the characters it matched. Then it matched the second
(negative) lookahead with the same 2-space. But because it's negative,
the whole match failed.

We just have to circumscribe the 2nd lookahead (negative) within the
first one.

This catches the 2 stray corner cases that I wanted to fix before
merging this branch: one line each in chown and npm.

But it also adds a hell of a regression in that it now matches plain
text from a lot of help messages. But this can be fixed easily enough,
cf. next commit.
The pattern we incorrectly matched was lines including some kind of
justified text, with ".  "

We can safely match this pattern out.
@victor-gp victor-gp mentioned this pull request Jul 31, 2023
@victor-gp victor-gp marked this pull request as ready for review July 31, 2023 01:02
@victor-gp victor-gp merged commit 143fffb into main Jul 31, 2023
3 checks passed
@victor-gp victor-gp deleted the scope-subcommand-followed-by-args branch July 31, 2023 02:04
victor-gp added a commit to victor-gp/bat that referenced this pull request Dec 29, 2023
Manual update (as opposed to Dependabot's) because the highlighting for
the test help message has changed. It's all good because it's as
intended, an improvement.

See victor-gp/cmd-help-sublime-syntax#23
victor-gp added a commit to victor-gp/bat that referenced this pull request Dec 29, 2023
Manual update (as opposed to Dependabot's) because the highlighting for
the test help message has changed. It's all good because it's as
intended, an improvement.

See victor-gp/cmd-help-sublime-syntax#23
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.

Wrong adb --help
1 participant