[#954] parser.rs: enable multiline descriptions #962

gabriel-vanzandycke · 2025-03-20T13:21:28Z

This PR adds the possibility to split description into multiple lines.
Resolves #954

The following cheat sheet:

% test
# Single line description
echo "hello world"
# Multilines
# description 1
```
echo -n hello
echo    world
```
# Multilines
# description 2
echo "hello world"
# Single line description
```
echo -n hello
echo    world
```

Yields the following result:

welcome · 2025-03-20T13:21:31Z

Thanks for opening this pull request!

…g on item.comment

gabriel-vanzandycke · 2025-03-20T15:42:23Z

I'm working on the multi-line code-block and multi-line description. Here is how I would like it to render

I'm almost there. The tag, description and snippet column are correctly rendered, but I struggle with the next columns (in terminal.rs). Can someone help me understand the purpose of the DELIMITER and what is expected by fzf?

kit494way · 2025-03-20T23:55:36Z

I'm not sure I understand your question correctly, but I will comment on what I understand about DELIMITER.
DELIMITER is a field separator that separates tags, descriptions, commands, and other fields.
DELIMITER is passed to fzf's --delimiter option.

navi/src/finder/mod.rs

Lines 113 to 114 in 6f1bbcf

    
           "--delimiter", 
        
           deser::terminal::DELIMITER.to_string().as_str(),

In manual of fzf:

-d, --delimiter=STR
Field delimiter regex for --nth, --with-nth, and field index expressions (default: AWK-style)

DELIMITER is used when we change the order of columns by passing --with-nth option to fzf.
For example, without --with-nth:

With --with-nth:

Does this help?

…locks with multiple lines (only fzf supported)

gabriel-vanzandycke · 2025-03-22T09:34:19Z

It did. Thanks!! I hadn't realized that the delimiter could be used for multiple columns within fzf. I'm updating this PR description to integrate latest changes.

alexis-opolka · 2025-03-24T00:38:17Z

It looks good @gabriel-vanzandycke , thanks!
I will let either @kit494way or @denisidoro review the code.

Could you link this PR to your issue ?
Using GitHub link words: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue

gabriel-vanzandycke · 2025-03-24T08:35:01Z

Caveats of this PR:
This implementation required to merge the 3 columns (tag, description and command). Therefore, it is no longer possible to change their order by just overriding fzf arguments (with --with-nth=3,2,1 for instance).

alexis-opolka · 2025-03-25T08:09:14Z

Personally, I don't think it's going to be a big issue, it would be a breaking change for sure to people mainly using this feature but it might make sense if we're moving forward to allow converting other cheatsheet types into navi cheatsheets (see #891) and introducing a syntax parser for cheatsheets (see #948).

To me, we can take two roads from here regarding the columns, both are breaking changes with the current implementation:

We still allow them to be "dynamic" and introduce in this PR another way to change their orders.
We decide they are fixed from now on and their orders won't change for the foreseeable future.

The second option would be viable IF modifying the order of the columns is not something a lot of users do.
I can't say since I don't use them in navi but I can think that there is people who might use them.

gabriel-vanzandycke · 2025-03-25T17:37:45Z

Implementing the first option doesn't necessarily need to be done in this PR as it implies a breaking change anyway...

kit494way

I think the change in the parsing multiline descriptions is compatible, but the change in the display part is a breaking change.
I think it is better to avoid this breaking change.
How about adding an option to display descriptions and snippets on multiple lines.

To add a note about the --with-nth option, it can be used not only for reordering columns but also for selecting which columns to display.

src/parser.rs

src/finder/mod.rs

gabriel-vanzandycke · 2025-03-26T08:32:45Z

Thanks @kit494way for your review !

How about adding an option to display descriptions and snippets on multiple lines.

An option specific for multilines rendering is anyway mutually exclusive with column selection option --with-nth= of fzf, and I'm not sure handling both cases in the code is best approach.

I believe giving users the ability to select columns another way is the best option in term of code maintenance, but it's a breaking change.

alexis-opolka · 2025-03-26T08:47:43Z

Implementing the first option doesn't necessarily need to be done in this PR as it implies a breaking change anyway...

I agree but it would be best to decide how we're going to change this for users before breaking the current behavior.
I don't want us to introduce breaking changes without knowing where we're going with it and I'm not going to merge this PR until then.

gabriel-vanzandycke · 2025-03-26T09:57:23Z

Can someone assist me for setting up tests ?

alexis-opolka · 2025-03-26T10:47:00Z

I won't be able to help you on that but maybe those references can help:

kit494way · 2025-03-26T23:29:10Z

@gabriel-vanzandycke

Can someone assist me for setting up tests ?

Can't run cargo test?
Or are you trying to run ./tests/run?
I think that ./tests/run is intended to be executed in an isolated environment like GitHub Actions, because the test kills tmux sessions and reads user config.yaml if NAVI_CONFIG is not properly set.
If you want to run ./tests/run locally, you might want to consider using act, although I have never run the test with act, so I can't confirm if it works.

alexis-opolka · 2025-03-27T07:56:44Z

If needed, I can trigger the tests on GitHub, just let me know.

gabriel-vanzandycke · 2025-03-27T09:47:09Z

src/parser.rs

@@ -258,14 +258,15 @@ impl<'a> Parser<'a> {

            // blank
            if line.is_empty() {
-                if !item.snippet.is_empty() {
+                if !item.snippet.is_empty() && inside_snippet {
                    item.snippet.push_str(deser::LINE_SEPARATOR);
                }
            }
            // tag
            else if line.starts_with('%') {
                should_break = self.write_cmd(&item).is_err();
                item.snippet = String::from("");


Can someone enlighten me on why the item is reset rather than creating a new item object when we move to the next item ? (It makes me wonder each time I'm in this file)

gabriel-vanzandycke · 2025-03-27T10:02:06Z

Thanks. cargo test passes on my side. I failed setting-up act, I'll investigate depending on the next GitHub actions results (@alexis-opolka ).

alexis-opolka · 2025-03-27T10:06:50Z

Thanks. cargo test passes on my side. I failed setting-up act, I'll investigate depending on the next GitHub actions results (@alexis-opolka ).

If you successfully installed act, you just need to be in the directory of navi (where .github is present) and run:

act push

It should trigger both CI/Lints and CI/Tests.

gabriel-vanzandycke · 2025-03-27T10:29:32Z

Thanks. I ran cargo fmt and pushed the changes, CI should now pass.

alexis-opolka · 2025-03-28T20:29:38Z

Great @gabriel-vanzandycke !

I am waiting for the approval of @kit494way and @denisidoro on this PR and since it contains breaking changes I don't know if we wait to merge it in a later major release or we upgrade the next release from a minor to a major release.

I would like to at least merge it after #955 .

EDIT: When I say a major release I mean a release with breaking changes.

kit494way · 2025-03-29T02:00:18Z

Please make displaying multiple lines optional.
I'd like to display the search table compactly.

kit494way · 2025-03-29T02:18:12Z

src/finder/mod.rs

@@ -119,7 +119,7 @@ impl FinderChoice {
        ]);

        if !opts.show_all_columns {
-            command.args(["--with-nth", "1,2,3"]);
+            command.args(["--read0", "--print0", "--with-nth", "1"]);


It is better to rename show_all_columns or introduce another option for --read0 and --print0 because --read0 and --print0 do not control the number of columns to display.

denisidoro · 2025-04-08T19:15:53Z

I agree with @kit494way. Let's make this opt-in. Otherwise, people may import cheatsheets using this syntax and be surprised by the behavior.

I'll approve the PR once this is addressed.

alexis-opolka · 2025-05-02T18:08:28Z

Hi @gabriel-vanzandycke ,
Have you had time to look into the requested changes in this PR?

gabriel-vanzandycke · 2025-05-02T20:10:48Z

Hello, no, I haven't had the time to look into this, sorry.

alexis-opolka · 2025-05-02T20:14:09Z

No worries. ^^

I just wanted to check on you, you can take the time you need to work on this PR.
Let us know if you need help, I might not be able to help much but I'm sure @kit494way or @denisidoro would help you if you need.

[denisidoro#954] parser.rs: enable multiline descriptions

07c2cf9

gabriel-vanzandycke requested a review from denisidoro as a code owner March 20, 2025 13:21

[denisidoro#954] terminal.rs: handle multiline descriptions by callin…

4ce58fd

…g on item.comment

gabriel-vanzandycke force-pushed the multiline-description branch from bd9876f to 4ce58fd Compare March 20, 2025 13:33

[denisidoro#954] Display multi-line description and multi-line code-b…

46e9f74

…locks with multiple lines (only fzf supported)

gabriel-vanzandycke added 3 commits March 22, 2025 10:52

[denisidoro#954] revert

b1cc46d

[denisidoro#954] remove un-necessary variable and eprintln

01e4600

[denisidoro#954] Do not collapse blank lines inside snippets

57c7c90

gabriel-vanzandycke force-pushed the multiline-description branch from 3d8cb7b to 57c7c90 Compare March 22, 2025 10:00

kit494way reviewed Mar 25, 2025

View reviewed changes

src/parser.rs Outdated Show resolved Hide resolved

src/finder/mod.rs Outdated Show resolved Hide resolved

src/finder/mod.rs Outdated Show resolved Hide resolved

fix variable support

643e74a

gabriel-vanzandycke force-pushed the multiline-description branch from 2e0c169 to 643e74a Compare March 26, 2025 09:28

comply with cargo format

23f5bf6

fix comment after new tag

9f2e455

revert to LINE_SEPARATOR added for each blank space

4b4f8d9

gabriel-vanzandycke commented Mar 27, 2025

View reviewed changes

cargo fmt

65bb845

alexis-opolka requested a review from kit494way March 28, 2025 20:29

alexis-opolka assigned gabriel-vanzandycke Mar 28, 2025

alexis-opolka added the new feature New feature or request label Mar 28, 2025

kit494way reviewed Mar 29, 2025

View reviewed changes

alexis-opolka mentioned this pull request Apr 8, 2025

Navi v2.25.0 #973

Open

alexis-opolka added the wip Work in progress label Apr 24, 2025

[#954] parser.rs: enable multiline descriptions #962

Are you sure you want to change the base?

[#954] parser.rs: enable multiline descriptions #962

Conversation

gabriel-vanzandycke commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcome bot commented Mar 20, 2025

Uh oh!

gabriel-vanzandycke commented Mar 20, 2025

Uh oh!

kit494way commented Mar 20, 2025

Uh oh!

gabriel-vanzandycke commented Mar 22, 2025

Uh oh!

alexis-opolka commented Mar 24, 2025

Uh oh!

gabriel-vanzandycke commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexis-opolka commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabriel-vanzandycke commented Mar 25, 2025

Uh oh!

kit494way left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabriel-vanzandycke commented Mar 26, 2025

Uh oh!

alexis-opolka commented Mar 26, 2025

Uh oh!

gabriel-vanzandycke commented Mar 26, 2025

Uh oh!

alexis-opolka commented Mar 26, 2025

Uh oh!

kit494way commented Mar 26, 2025

Uh oh!

alexis-opolka commented Mar 27, 2025

Uh oh!

gabriel-vanzandycke Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabriel-vanzandycke commented Mar 27, 2025

Uh oh!

alexis-opolka commented Mar 27, 2025

Uh oh!

gabriel-vanzandycke commented Mar 27, 2025

Uh oh!

alexis-opolka commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kit494way commented Mar 29, 2025

Uh oh!

kit494way Mar 29, 2025

Choose a reason for hiding this comment

Uh oh!

denisidoro commented Apr 8, 2025

Uh oh!

alexis-opolka commented May 2, 2025

Uh oh!

gabriel-vanzandycke commented May 2, 2025

Uh oh!

alexis-opolka commented May 2, 2025

Uh oh!

Uh oh!

gabriel-vanzandycke commented Mar 20, 2025 •

edited

Loading

gabriel-vanzandycke commented Mar 24, 2025 •

edited

Loading

alexis-opolka commented Mar 25, 2025 •

edited

Loading

gabriel-vanzandycke Mar 27, 2025 •

edited

Loading

alexis-opolka commented Mar 28, 2025 •

edited

Loading