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

Chooser: Pass justfile path to preview command for default chooser #1759

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

Qeole
Copy link
Contributor

@Qeole Qeole commented Dec 12, 2023

The default chooser is fzf, and we pass it a command to generate the preview with just --show from the recipe name. This has been working well as long as we rely on the default justfile available in the directory, given that the preview command can find it too. However, passing --chose alongside --justfile to select a specific file results in the preview command not being able to find the right justfile to process.

To address this issue, we turn the const string defining the preview command into a function that takes the internal representation of the justfile as an argument, and updates the preview command with the right file.

Fixes: #1638

@Qeole Qeole force-pushed the pr/justfile-path-chooser-preview branch 2 times, most recently from a6c73d4 to c648b0d Compare December 12, 2023 21:55
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple small comments, otherwise looks good.

src/subcommand.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
@Qeole Qeole force-pushed the pr/justfile-path-chooser-preview branch from c648b0d to 505be59 Compare December 13, 2023 21:25
@Qeole
Copy link
Contributor Author

Qeole commented Dec 13, 2023

I'm getting a failure on the CI test this time, justfile has been replaced with /tmp/just-test-tempdirWEynpr/justfile. I can probably use a wildcard to address this in the test.

---- choose::invoke_error_function stdout ----
thread 'choose::invoke_error_function' panicked at tests/test.rs:244:9:
Stderr regex mismatch:
"error: Chooser `/ -cu fzf --multi --preview 'just --unstable --color always --justfile /tmp/just-test-tempdirWEynpr/justfile --show {}'` invocation failed: Permission denied (os error 13)\n"
!~=
/Regex("^error: Chooser `/ -cu fzf --multi --preview 'just --unstable --color always --justfile justfile --show \\{\\}'` invocation failed: .*\\n$")/

The default chooser is fzf, and we pass it a command to generate the
preview with "just --show" from the recipe name. This has been working
well as long as we rely on the default justfile available in the
directory, given that the preview command can find it too. However,
passing "--chose" alongside "--justfile" to select a specific file
results in the preview command not being able to find the right justfile
to process.

To address this issue, we turn the const string defining the preview
command into a function that takes the internal representation of the
justfile as an argument, and updates the preview command with the right
file.

Fixes: 5f9ac39 ("Use `just --show` in default chooser (casey#1539)")
Suggested-by: Casey Rodarmor <[email protected]>
@Qeole Qeole force-pushed the pr/justfile-path-chooser-preview branch from 505be59 to e06e12a Compare December 13, 2023 21:38
@Qeole Qeole requested a review from casey December 13, 2023 21:45
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM! I added quotes around the path in the chooser string, since it might wind up having spaces.

@casey casey enabled auto-merge (squash) December 14, 2023 00:46
@casey casey merged commit 3dbbb2e into casey:master Dec 14, 2023
5 checks passed
@Qeole Qeole deleted the pr/justfile-path-chooser-preview branch December 14, 2023 01:36
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.

--choose side pane not using correct just file
2 participants