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

pass fzf query or cmd match to editor environment #361

Closed
wants to merge 1 commit into from

Conversation

khimaros
Copy link
Contributor

developed this as a way of scratching a personal itch: #330

see the updates to docs/tool-editor.md for sample usage

i'm happy to incorporate feedback to make sure this fits cleanly

@khimaros khimaros force-pushed the main branch 4 times, most recently from 0cd2b38 to 93ee42e Compare November 23, 2023 13:25
@khimaros khimaros changed the title [WIP] pass fzf query or cmd match to editor environment pass fzf query or cmd match to editor environment Nov 23, 2023
@khimaros
Copy link
Contributor Author

@mickael-menu i think this is now ready for review

Copy link

This pull request has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale No recent activity label Dec 24, 2023
@tjex
Copy link
Member

tjex commented Jan 11, 2024

Hi @khimaros , just to update you, it would seem that your pr caught mickaël at the tail end of his passing on of zk.
The project is currently being transferred over to a new group of maintainers.

See the readme for the notice. Thanks for the pr, once we're out of maintenance mode, let's look at this again :)

@khimaros
Copy link
Contributor Author

@tjex thank you and looking forward to it

@github-actions github-actions bot removed the stale No recent activity label Jan 12, 2024
Copy link

This pull request has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale No recent activity label Feb 12, 2024
@khimaros
Copy link
Contributor Author

@tjex FYI

@github-actions github-actions bot removed the stale No recent activity label Feb 13, 2024
@tjex
Copy link
Member

tjex commented Feb 13, 2024

Is it ok if we close this for now? So we don't keep getting pinged by the github-actions notifier? We've still got some bugs to sort out and I'm also leaving on holiday from tomorrow for a month.
If you'd like to make a case for it getting merged in the meantime, you could try pinging one of the other maintainers, but we have all agreed to not look at any new features until the bugs are sorted.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale No recent activity label Mar 15, 2024
@khimaros
Copy link
Contributor Author

i've rebased this pull request on main. i think this is still a valuable addition to zk and i use it as part of my daily note taking workflow.

@github-actions github-actions bot removed the stale No recent activity label Apr 24, 2024
@tjex
Copy link
Member

tjex commented Apr 24, 2024

Thanks for pinging back. I've had a look through and a think, and I'm unsure whether the added complexity in the code base is merited for this feature to be implemented in the main fork. Specifically, I'm thinking of:

  • requiring the passing of an empty string as a second argument in NewEditor(opt.NullString, "") and container.NewNoteEditor(notebook, "")
  • changing test cases
  • requiring the setting of a bash script as editor for this to work

While opening at a specific line is useful, I feel that doing so from the CLI isn't so broadly needed when the term can be quickly searched for in the file, post opening. Additionally, I would think that the majority of users will be using zk from within an editor whereby LSP and grep tooling (telescope.nvim etc) provides a lot of power already.

Do @zk-org/maintainers have any contrary thoughts?

@mcDevnagh
Copy link
Contributor

So what is the use case here? You run zk edit -i type in a query, and then you receive that query as an OS environment variable?

@khimaros
Copy link
Contributor Author

khimaros commented May 3, 2024

@mcDevnagh my use case was to have vim automatically jump to the line that was selected in zk. however, the way i implemented this is flexible and could be used for other purposes. background: #330

Copy link

github-actions bot commented Jun 2, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale No recent activity label Jun 2, 2024
@khimaros
Copy link
Contributor Author

khimaros commented Jun 2, 2024

final ping @mcDevnagh

@github-actions github-actions bot removed the stale No recent activity label Jun 3, 2024
@mickael-menu
Copy link
Member

@zk-org/maintainers If you need to alter the stale/triage rules, it's here:

days-before-pr-close: -1

Note that PRs are never automatically closed, only issues, so IMHO that's fine to leave it tagged as stale until someone decides to address it.

Copy link

github-actions bot commented Jul 5, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale No recent activity label Jul 5, 2024
@tjex
Copy link
Member

tjex commented Jul 5, 2024

@khimaros I think it's gotten to the point now where we should pass on this PR. From my side, I'm resistant to merging it as it introduces a fair amount of instability in the codebase / user config by a) adding an extra argument to NewEditor() that will seemingly only be required once (in edit.go) and b) the implementation on the user side with the script is quite involved and very prone to user side issue. So for both code and user config, I see quite a bit of potential for issue and setup / maintenance work with not much value gain.

I say "not much value gain" because there is also a PR over on zk-nvim (zk-org/zk-nvim#171) which is implementing a grep feature that also uses zk list --match in the back end. So the functionality will be effectively the same, albeit the user needs to already be in the editor (and it's neovim specific).

Furthermore, on a more stickler note, notes in Zettelkaten notes should be atomic, meaning there shouldn't be much text and therefore it shouldn't be hard to find the word or phrase which resides inside the returned note.

I tried to have it otherwise by pinging the other maintainers, but in the end it looks like my call... Sorry! But thanks so much for the idea and patience.

@tjex tjex closed this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants