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

supporting piping a buffer to an external process #1204

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

fischerling
Copy link
Contributor

@fischerling fischerling commented Sep 8, 2024

Alternative to #1177.

There are two major differences:

  • No new lua function is introduced. vis:pipe("string", cmd) is used instead.
  • No memory allocations are required.

vis_pipe is extended to support both file and range as well as buffer arguments.
Wrapper functions are provided to use the old behavior and the new one with a clean interface.

@rnpnr
Copy link
Collaborator

rnpnr commented Sep 8, 2024

Sound good to me! I won't have time to review this until Friday but it seems reasonable given that lua already has the string allocated.

Also thanks for adding a test! I need to remember to install busted in the github workflow so that the lua tests actually run.

@fischerling
Copy link
Contributor Author

Also thanks for adding a test! I need to remember to install busted in the github workflow so that the lua tests actually run.

I added busted to the workflows in #1205.

Currently only Text objects can be piped to external commands.
This is tedious if data not available in any file should be passed
to an external process (e.g. building options and passing them to
vis-menu).

This adds the option to pass a buffer to _vis_pipe and provides wrapper
functions for the original behavior and the new one.
Support the old behavior of using vis:pipe(cmd, fullscreen) without
input.
Properly distinguish between vis:pipe(text, cmd, fullscreen) and
vis:pipe(file, range, cmd).
Copy link
Collaborator

@rnpnr rnpnr left a comment

Choose a reason for hiding this comment

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

This all looks good to me and all the tests are great! The vis:pipe is one of the more complicated ones on the C side so its good to make sure all variations are working.

I'm merging with a few minor style tweaks and I squashed the test commits into one.

Thanks for the patch!

@rnpnr rnpnr merged commit c8694ee into martanne:master Sep 13, 2024
19 checks passed
@mcepl
Copy link
Contributor

mcepl commented Sep 23, 2024

BTW, does this fix also https://todo.sr.ht/~martanne/vis/5 ?

@fischerling
Copy link
Contributor Author

BTW, does this fix also https://todo.sr.ht/~martanne/vis/5 ?

Probably yes.

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.

3 participants