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

[RFC] Add fzf support #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Holzhaus
Copy link

@Holzhaus Holzhaus commented Dec 7, 2018

A quick and dirty implementation to select a query result (#37).

A quick and dirty implementation to select a query result (sunaku#37).
@Holzhaus Holzhaus changed the title [FZF] Add fzf support [RFC] Add fzf support Dec 7, 2018
@@ -0,0 +1,107 @@
#!/bin/sh -e
#
# # DASHT-QUERY-HTML 1 2018-10-09 2.3.0
Copy link
Owner

Choose a reason for hiding this comment

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

All mentions of the old dasht-query-html should be changed to dasht-query-fzf.

if (pattern == "") pattern = "^." # grouped by leading character
}
NR == 1 {}
$2 == "=" { result[$1] = substr($0, index($0, $2) + length($2) + 1) }
Copy link
Owner

Choose a reason for hiding this comment

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

We need to share this AWK preamble (all of the lines above this one) with the dasht-query-html script to avoid copy/paste duplication. 🤔 Perhaps we can write a separate script that just does this preamble and emits the search results in an intermediate format, which the *-html and this *-fzf scripts can then transform into their final output formats.

@sunaku
Copy link
Owner

sunaku commented Dec 15, 2018

Thanks for the PR 👍 I'll merge it after some clean up. 😅

@Holzhaus
Copy link
Author

Thanks for the PR I'll merge it after some clean up.

Ah cool, I didn't expect it was already good enough to be merged. IMHO it's a bit confusing that dasht-query-html returns html code but dasht-query-fzf just returns a file URL, which also causes duplication in the dasht script. However, I didn't find a way to scroll to the correct anchor in w3m when piping output to it, so maybe there is no better way.

@smackesey
Copy link
Contributor

smackesey commented Oct 26, 2020

Before realizing that there was an existing PR for this, I wrote a version myself that has some advantages and disadvantages vis-a-vis this one. My version:

  • (+) emits HTML instead of just a URL from dasht-query-fzf emits the same link HTML used in dasht-query-html for a single result).
  • (+) does tabular alignment of the search results in fzf for greater readability
  • (+) has less code duplication in dasht
  • (-) lacks the text styling of this one

But IMO, rather than including fzf support directly within dasht, it would be good, as suggested by @sunaku above, to generate intermediate results in a standard format and expose this functionality directly to the user. That would make this tool more open-ended and easy for the user to adapt to their needs (following the unix philosophy of "do one thing well"), without creating an open-ended precedent/burden of maintenance for various "adapters".

So what if:

  • the intermediate result format is simply a tsv (\t-separated set of fields): name, docset, type, url
  • dasht-query-line is updated to output the intermediate result format
  • dasht-query-html is adjusted to remove the work now performed by dasht-query-line
  • main dasht executable emits the tsv results by default
  • main dasht executable takes a --browse option that triggers the current default behavior of browsing in w3m.
  • for consistency, scrap dasht-server and have main dasht executable take a --serve option that runs the code currently in dasht-server

It seems to me that outputting "neutral" query results by default would serve the typical user better, who wants to use dasht in some kind of editor integration pipeline etc. And unifying all the user facing scripts behind a single dasht executable is also cleaner.

I'm willing to implement the above changes if you're interested @sunaku.

@smackesey
Copy link
Contributor

Pinging @sunaku on this-- I've made most of the above changes to my private fork and can submit a PR if you are interested in changing the interface in this way.

@sunaku
Copy link
Owner

sunaku commented Jul 21, 2021

Hi @smackesey, I like your idea of emitting TSV as an intermediate format in #38 (comment). Please submit a PR for it, thanks.

@smackesey
Copy link
Contributor

@sunaku Two questions:

  • Do you want all the changes mentioned in my comment?
  • The implementation I've written includes the commits from my other PR, I think I addressed all the requested changes there, could you have a look and merge or let me know if it needs something else? Then I can submit the TSV PR.

@sunaku
Copy link
Owner

sunaku commented Jul 22, 2021

Hi @smackesey,

  1. Please include only these changes in the new TSV PR:
  • the intermediate result format is simply a tsv (\t-separated set of fields): name, docset, type, url
  • dasht-query-line is updated to output the intermediate result format
  • dasht-query-html is adjusted to remove the work now performed by dasht-query-line

Notably, please exclude these changes from the TSV PR:

  • main dasht executable emits the tsv results by default
  • main dasht executable takes a --browse option that triggers the current default behavior of browsing in w3m.
  • for consistency, scrap dasht-server and have main dasht executable take a --serve option that runs the code currently in dasht-server
  1. I previously added some more review comments to your other PR add support for using existing Dash.app docsets #53 but forgot to submit them. 😅 This is done correctly now, so you should see a "requested changes" status on the PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants