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

Replace Pipelines "Behind the Scenes" with "Result Display Rendering" #1635

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kissaki
Copy link
Contributor

@Kissaki Kissaki commented Nov 17, 2024

The previous "Behind the Scenes" docs has some problems:

  • The title is not descriptive
  • Confusing context reference: The "You may have wondered" references ls output despite no applicable example in front of it - and is not stable against other docs section changes either way
  • Confusing false equivalence: Claims "one and the same" and then explains how they are not in fact the same

The section is being replaced with "Result Display Rendering", which

  • Mentions and explains the existence of a display output hook
  • Begins with establishing context: The section is about interactive shell pipeline result display output

I kept the section small so as not to duplicate content from the Hooks documentation page.


Adding examples for current, different, or simplified displaying may be reasonable here, but would duplicate the hooks "Changing how Output is Displayed" and "Filtering or Diverting Command Output" sections.

@NotTheDr01ds
Copy link
Contributor

This looks like a good, straightforward simplification to me - Thanks!

@132ikl
Copy link
Contributor

132ikl commented Nov 17, 2024

As noted in nushell/nushell#14361, display_output is actually not what runs table, but instead table is automatically appended to printed pipelines. I think this should be mentioned in the docs if we maintain this behavior, and shouldn't imply that display_output does everything.

Copy link
Contributor

@NotTheDr01ds NotTheDr01ds left a comment

Choose a reason for hiding this comment

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

@132ikl Does that suggestion look better?

Comment on lines +358 to +385
In interactive mode, when a pipeline ends, the [`display_output` hook configuration](https://www.nushell.sh/book/hooks.html#changing-how-output-is-displayed) defines how the result will be displayed.
The default configuration uses the [`table` command](/commands/docs/table.md) to render structured data as a visual table.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In interactive mode, when a pipeline ends, the [`display_output` hook configuration](https://www.nushell.sh/book/hooks.html#changing-how-output-is-displayed) defines how the result will be displayed.
The default configuration uses the [`table` command](/commands/docs/table.md) to render structured data as a visual table.
In interactive mode, two additional expressions are added to the end of each pipeline to ensure proper rendering of the result:
1. First, if defined, the [`display_output` hook](https://www.nushell.sh/book/hooks.html#changing-how-output-is-displayed) is added. This allows the user to change the rendering.
2. Nushell then internally adds a `table` command to render the final result.

@NotTheDr01ds
Copy link
Contributor

You're probably right - When I look at it in light of that, it may be an over-simplification. Does my suggested edit about look more accurate?

@Kissaki
Copy link
Contributor Author

Kissaki commented Nov 18, 2024

@132ikl thank you for raising that concern. Although I tested with null or empty setting the hook, I assumed Nushell fell back to table in such cases. Both concepts behave practically the same, but of course it'd be better to document it as it is implemented.

Shall this PR wait for the discussion in nushell/nushell#14361 then, or make adjustments like @NotTheDr01ds suggested so the improvements can land? I don't know how long or not the referenced discussion may take?

@132ikl
Copy link
Contributor

132ikl commented Nov 19, 2024

@Kissaki nushell/nushell#14361 was landed, so I think your original explanation is good. Technically, if the hook is not set (or set to null), the table command is used as well, but I don't think that's really necessary to mention in the docs.

It could be useful to show what happens if you set $env.config.hooks.display_output = {||} and say something to the effect of "you can see that without the call to table, tables are really just a list of records," but feel free to take it whatever direction you want.

Kissaki and others added 4 commits February 2, 2025 17:06
The previous "Behind the Scenes" docs has some problems:

* The title is not descriptive
* Confusing context reference: The "You may have wondered" references `ls` output despite no applicable example in front of it - and is not stable against other docs section changes either way
* Confusing false equivalence: Claims "one and the same" and then explains how they are not in fact the same

The section is being replaced with a "Result Display Rendering", which

* Mentions and explains the existence of a display output hook
* Begins with establishing context: The section is about interactive shell pipeline result display output

---

I kept the section small as not to duplicate content from the Hooks documentation page.

Adding examples for current, different, or simplified displaying may be reasonable here, but would duplicate the hooks "Changing how Output is Displayed" and "Filtering or Diverting Command Output" sections.
Matching link elsewhere in the file
Co-authored-by: Douglas <32344964+NotTheDr01ds@users.noreply.github.com>
@Kissaki Kissaki force-pushed the docs/pipelines-displayoutput branch from a851b48 to cfe9430 Compare February 2, 2025 16:27
@Kissaki
Copy link
Contributor Author

Kissaki commented Feb 2, 2025

It seemed to me like there was nothing open here but nothing happened anymore. Anyway, I applied the suggested headline change, and added examples including behavior with an empty closure as suggested.

PTAL

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