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

:write_stdout and :writeln_stdout internals #901

Closed
wants to merge 3 commits into from
Closed

Conversation

Canop
Copy link
Owner

@Canop Canop commented Jul 8, 2024

The :writeln_stdout internal allows recreating the :print_path verb:

    {
        invocation: pp
        cmd: ":write_stdout {file};:quit"
    }

but it's more generic.

With the :write_stdout, you can for example write the path without the trailing newline:

    {
        invocation: wp
        cmd: ":writeln_stdout {file};:quit"
    }

@AeliusSaionji
Copy link

These are the verbs I tested against

      {
          invocation: "echo_stdout_quit"
          external: "echo {file}"
          from_shell: true
      }
      {
          invocation: "test_stdout_quit"
          cmd: ":write_stdout {file};:quit"
      }
      {
          invocation: "testln_stdout_quit"
          cmd: ":writeln_stdout {file};:quit"
      }
      {
          invocation: "test_stdout"
          cmd: ":write_stdout {file}"
      }
      {
          invocation: "testln_stdout"
          cmd: ":writeln_stdout {file}"
      }

I'm seeing a few oddities here.

In no particular order:

  1. Should write_stdout and writeln_stdout be callable from within broot? They don't accomplish much outside of verb use.
  2. Is this new? It's preventing use of @Alfamari 's previously working echo {file} from shell.
    image
  3. In the staging area, the four test* verbs don't trigger the above error, but they also simply aren't found if more than one file is staged.
    image
  4. Staging area would be preferred, since it allows the user to see what they have done, and allows for correcting mistakes. However, via testln_stdout it is possible to accumulate files on stdout. Unfortunately this smacks us right into the original problem from Output from Br shell function in powershell doesn't print to stdout #888 : there's a newline at the end of the list. Note: I reverted my changes in br.ps1 to test this PR.
    image
  5. With test_stdout_quit it is possible to pass one file to powershell.
    image
    But of course, multiple invocations of test_stdout won't help with further files.
    image
    I do see a workaround - calling testln_stdout for some files, and test_stdout_quit on the last file. I don't know that this qualifies as a solution though.
    image
  6. This might be an unrelated bug, but in the course of testing these verbs some dozen times I noticed that tab complete happily cycles through all four verbs with test<tab>, but testln<tab> autocompletes to testln_stdout_quit and will not cycle with the other match testln_stdout. 🤷

In summary, write_stdout and writeln_stdout narrowly avoid solving the problem :)

  • write_stdout only supports passing one file
  • writeln_stdout always contains a problematic newline
  • Neither are available in the staging area

As a final thought - in addition to text output, it might be worth supporting json output, too. That's a trendy thing to do these days, and would play quite nicely with object oriented shells like powershell. Or any shell could leverage something like jq.

@Canop
Copy link
Owner Author

Canop commented Jul 12, 2024

A lot of good point.
Also #888 (comment)

@Canop
Copy link
Owner Author

Canop commented Jul 12, 2024

An approach might be to allow more settings to be carried by verb arguments, using the syntax I introduced at https://github.com/Canop/broot/blob/main/CHANGELOG.md#v091---2019-07-29

This could for example be write_stdout {file:comma-separated};:quit or write_stdout {directory:json-array} or write_stdout {directory:lines}.

I'll be cautious though, as I don't want to be carried into introducing user-side complexity solving only one problem.

@Canop
Copy link
Owner Author

Canop commented Aug 31, 2024

I'm dropping this PR and will try another approach.

@Canop Canop closed this Aug 31, 2024
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.

2 participants