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.

Sign up for GitHub

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

Jump to bottom

update dasht-query-line and dasht-query-html to work with tsv #56

Closed
wants to merge 7 commits into from
Closed

update dasht-query-line and dasht-query-html to work with tsv #56

wants to merge 7 commits into from

Conversation

smackesey
Copy link
Contributor

I extracted my work on TSV from the other Dash.app integration fork so that this could be separately merged. It follows your requests in #38.

sunaku
sunaku requested changes Aug 5, 2021
View reviewed changes
Copy link
Owner

@sunaku sunaku left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I've requested some minor changes in the code review.

bin/dasht-query-line Outdated Show resolved Hide resolved
bin/dasht-query-line Outdated
@@ -28,8 +28,8 @@
#
# Searches for *PATTERN* in all installed [Dash] docsets, optionally searching
# only in those whose names match *DOCSET*s, by calling dasht-query-exec(1)
# and emits the results in groups of lines, as described in "Results" below.
# However, if no results were found, this program exits with a nonzero status.
# and emits the results as TSV. However, if no results were found, this program
Copy link
Owner

Choose a reason for hiding this comment

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

Please spell out "TSV" in prose: "as TSV" -> "in Tab Separated Vector (TSV) format"

bin/dasht-query-line Outdated
@@ -42,26 +42,24 @@
#
# ### Results
#
# Each search result is printed to stdout as a group of four lines of text:
# Each search result is printed to stdout as a tab-separated line with fields:
Copy link
Owner

Choose a reason for hiding this comment

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

The fields are separated, not the line; so "as a line with 4 tab-separated fields".

bin/dasht-query-line Outdated
@@ -245,9 +243,11 @@ dasht-docsets "$@" | while read -r docset; do

{ $1 = $1 } # strip whitespace from key

$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.

I would change the index($0, $2) call to length($1) + 1 for robustness (no problem if $1 contains =).

bin/dasht-query-line Outdated

/./ # reject any empty lines from input
# print TSV line
Copy link
Owner

Choose a reason for hiding this comment

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

I would drop this (redundant) comment.

bin/dasht-query-line Outdated

/./ # reject any empty lines from input
# print TSV line
printf "%s\t%s\t%s\t%s\n", result["name"], docset, \
Copy link
Owner

Choose a reason for hiding this comment

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

I would add parentheses to the printf() call and list each argument on its own line.

@smackesey
Copy link
Contributor Author

Made the changes!

@sunaku sunaku self-assigned this Aug 25, 2021
sunaku
sunaku requested changes Aug 26, 2021
View reviewed changes
Copy link
Owner

@sunaku sunaku left a comment

Choose a reason for hiding this comment

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

Thanks @smackesey! I found some time to experiment with your latest changes and I would suggest applying the changes in the attached patch file in addition to my code review feedback.

This PR is coming along nicely and we'll reach the finish line soon! 😅 Thanks for your patience.

bin/dasht-query-html Outdated Show resolved Hide resolved
bin/dasht-query-line Outdated Show resolved Hide resolved
@smackesey
Copy link
Contributor Author

smackesey commented Aug 26, 2021 •
edited
Loading

Thanks! So I made the changes you listed, but I was unable to apply the patch. I've never applied a patch file to a git repo before so perhaps I was doing something wrong, but I tried git apply /path/to/patch-file and got an error message:

error: patch failed: bin/dasht-query-line:261
error: bin/dasht-query-line: patch does not apply

sunaku
sunaku requested changes Aug 27, 2021
View reviewed changes
Copy link
Owner

@sunaku sunaku left a comment

Choose a reason for hiding this comment

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

Thanks for the update. There's a potential AWK incompatibility that I've called out in the code review. No worries about the patch file: I'll apply it on my side when I finally merge the PR. 🤓

bin/dasht-query-html Outdated Show resolved Hide resolved
sunaku
sunaku requested changes Aug 27, 2021
View reviewed changes
bin/dasht-query-line Outdated Show resolved Hide resolved
@sunaku
Copy link
Owner

sunaku commented Aug 28, 2021

Looks great, thanks! I'll try this out for a few days and let you know. Overall, we should be good to merge. 👍

@sunaku
Copy link
Owner

sunaku commented Aug 30, 2021

Hi @smackesey, I found an issue when searching the BASH docset:

  1. Before this PR
$ dasht 1 bash
digit-argument (M-0, M-1, … M--)  Bash function
PS1                               Bash variable
self-insert (a, b, A, 1, !, …)    Bash function
  1. After this PR:
$ dasht 1 bash
digit-argument  (M-0, m-1,
PS1              Bash variable
self-insert       (a, b,

Following the first result link shows a Can't load %E2%80%A6 error in w3m.

@smackesey
Copy link
Contributor Author

Fixed, the issue was that the awk field separator was still set to the default instead of tab. It didn't affect without spaces in the name, but screwed up those like the ones you posted. Sorry about that.

@sunaku
Copy link
Owner

sunaku commented Oct 2, 2021

Sorry for the long delay. 😅 I'll merge this PR soon (ETA this week).

sunaku added a commit that referenced this pull request Oct 30, 2021
@sunaku
Merge pull request #56 from smackesey/tsv-output
49320ba 
Switch to Tab-Separated Values (TSV) format in dasht-query-line(1) and
dasht-query-html(1) for easier line-by-line parsing and user scripting.
@sunaku
Copy link
Owner

sunaku commented Oct 30, 2021 •
edited
Loading

Merged in commit 882c7f2. Thanks for your contribution 🙏 and sorry for the long delay! 😅

@sunaku sunaku closed this Oct 30, 2021
sunaku added a commit that referenced this pull request Oct 30, 2021
@sunaku
Merge pull request #56 from smackesey/tsv-output
882c7f2 
Switch to Tab-Separated Values (TSV) format in dasht-query-line(1) and
dasht-query-html(1) for easier line-by-line parsing and user scripting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunaku sunaku sunaku requested changes

Assignees

@sunaku sunaku

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smackesey @sunaku