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

Add more tests of placeholder flags and simplify its logic #2624

Merged
merged 8 commits into from Oct 15, 2021

Conversation

vovcacik
Copy link
Contributor

@vovcacik vovcacik commented Oct 5, 2021

Hi, this is some work I have done while playing with #2609. No changes though, just tests, refactoring and bit of docs.

Notable:

  • Most of the value is in the tests that improve the coverage of placeholder flags.
  • But I have also refactored replacePlaceholder function, that seemed rather difficult to understand/edit at first. I have got tests before I touched anything; all the logic should be equivalent.

This tests some reasonable commands in fzf's templates (for commands,
previews, rebinds etc.), how are those commands escaped (backslashes,
double quotes), and documents if the output is executable in cmd.exe.
Both on Unix and Windows.
Adds tests and bit of docs for the curly brackets placeholders in fzf's
template strings. Also tests the "placeholder" regex.
Replacing placeholders in templates is already tested, this adds tests
that focus more on the parameters of placeholders - e.g. flags, token
ranges.

There is at least one test for each flag, not all combinations are
tested though.
Quotes and a typo.
…g source file

This is minor refactoring, and also the function's test was made
crossplatform.
Should be equivalent to the original, but has simpler structure.
@vovcacik
Copy link
Contributor Author

vovcacik commented Oct 5, 2021

(the failing test should be just a hiccup, I've run the workflows successfully before PR)

src/terminal.go Outdated Show resolved Hide resolved
src/terminal.go Outdated Show resolved Hide resolved
@junegunn junegunn merged commit 61339a8 into junegunn:master Oct 15, 2021
@junegunn
Copy link
Owner

Merged, thanks!

@vovcacik
Copy link
Contributor Author

Cool, thanks for merging.

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.

None yet

2 participants