[#954] parser.rs: enable multiline descriptions

[gabriel-vanzandycke]

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]

Thanks for opening this pull request!

[gabriel-vanzandycke]

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]

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?

[gabriel-vanzandycke]

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]

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]

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]

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:

1. We still allow them to be "dynamic" and introduce in this PR another way to change their orders.
2. 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]

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.

[gabriel-vanzandycke]

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]

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]

Can someone assist me for setting up tests ?

[alexis-opolka]

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

• https://doc.rust-lang.org/book/ch11-01-writing-tests.html
• https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html

[kit494way]

@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]

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

[gabriel-vanzandycke]

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]

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]

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]

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

[alexis-opolka]

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]

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

[kit494way]

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]

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]

Awesome! (^-^)

I'll take a look at it during the day and launch the workflow.

[gabriel-vanzandycke]

I can't reproduce the failing test, but encounter another error:

./tests/run
3rd party - tldr
Hey, listen! navi encountered a problem.
Do you think this is a bug? File an issue at https://github.com/denisidoro/navi.

Caused by:
    Failed to call:
    tldr docker --markdown
    
    Output:
    
    
    Error:
    error: unexpected argument '--markdown' found
    
      tip: to pass '--markdown' as a value, use '-- --markdown'
    
    Usage: tldr <COMMAND>...
    
    For more information, try '--help'.
    
    
    Note:
    The client.tealdeer config option can be set to enable tealdeer support.
    If you want to use another client, please make sure it supports the --markdown flag.
    If you are already using a supported version you can ignore this message.
    tldr-c-client (the default one in Homebrew) doesn't support markdown files, so navi can't use it.
    The recommended client is tealdeer(https://github.com/dbrgn/tealdeer).
    

Stack backtrace:
   0: anyhow::error::<impl core::convert::From<E> for anyhow::Error>::from
             at /home/gabriel/.cargo/registry/src/index.crates.io-6f17d22bba15001f/anyhow-1.0.100/src/backtrace.rs:27:14
   1: <T as core::convert::Into<U>>::into
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/convert/mod.rs:759:9
   2: navi::main::{{closure}}
             at ./src/bin/main.rs:34:9
   3: core::result::Result<T,E>::map_err
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/result.rs:854:27
   4: navi::main
             at ./src/bin/main.rs:32:5
   5: core::ops::function::FnOnce::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5
   6: std::sys::backtrace::__rust_begin_short_backtrace
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/sys/backtrace.rs:152:18
   7: std::rt::lang_start::{{closure}}
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/rt.rs:162:18
   8: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:284:13
   9: std::panicking::try::do_call
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:557:40
  10: std::panicking::try
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:521:19
  11: std::panic::catch_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panic.rs:350:14
  12: std::rt::lang_start_internal::{{closure}}
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/rt.rs:141:48
  13: std::panicking::try::do_call
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:557:40
  14: std::panicking::try
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:521:19
  15: std::panic::catch_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panic.rs:350:14
  16: std::rt::lang_start_internal
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/rt.rs:141:20
  17: std::rt::lang_start
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/rt.rs:161:17
  18: main
  19: __libc_start_call_main
             at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  20: __libc_start_main_impl
             at ./csu/../csu/libc-start.c:392:3
  21: _start
✖ Expected '' to include 'docker p'
✖ Test failed...

tldr --version
tealdeer 1.8.0

I don't understand how to get the correct tldr version, it seems I have the latest.

[alexis-opolka]

Hmm...

Right now, I don't know, I launched the CI again to see the latest result after your commits.
I know we had an issue with tldr since Navi doesn't integrate well with tldr clients, I'll try to investigate it later and come back to you.

[alexis-opolka]

Hi, I have an idea of what could be going wrong with this test and it doesn't seem to affect much of the application so I think I'll merge your PR and tackle this issue down afterwards since I also need to stabilise experimental before merging for the next pre-release.

So now, I'm just waiting @kit494way review before merging. (Maybe even @denisidoro if he wishes to.)

[kit494way]

Comments are not displayed correctly when a snippet follows a variable.
It includes comments from the previous snippet.

this branch:

master:

I tested it with this snippet.

navi/tests/no_prompt_cheats/cases.cheat

Lines 64 to 82 in b01291e

	# helper -> "inside helper: 42"
	myhelperfn 42
	$ x: echo '2'
	$ x2: echo "$((x+10))"
	$ y: echo 'a'
	$ language: echo '0 rust rust-lang.org' --- --column 2
	$ language2: echo '1;clojure;clojure.org' --- --column 2 --delimiter ';'
	$ multiword: echo 'foo bar'
	$ pictures_folder: echo "/my/pictures"
	$ map1: echo "foo" --- --map 'echo _$(cat)_'
	$ multilinevar: echo "xoo yar" \
	| tr 'x' 'f' \
	| tr 'y' 'b'
	$ expand1: echo "foo" --- --expand
	# this should be displayed -> "hi"
	echo hi

[alexis-opolka]

@gabriel-vanzandycke Have you got any time to check on @kit494way feedback?

If not, no worries. ^-^

[alexis-opolka]

Hi @gabriel-vanzandycke ,
I hope you're doing well! Would you mind if I take over your work on a dedicated branch in order to be able to merge the changes for the next version of navi?

Of course, you'll be put as a reviewer when the PR will be put to merge the feature onto the main branch.

[gabriel-vanzandycke]

Oh, thank you for the follow-up ! Yes, be my guest ! I'm pretty busy on other projects right now but I can definitively review the MR!

[welcome]

Congrats on merging your first pull request!