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

Initial $in language reference #1398

Merged
merged 3 commits into from
May 28, 2024
Merged

Conversation

NotTheDr01ds
Copy link
Contributor

Here's a first pass at $in documentation for the Language Reference Guide. Let me know if I missed any nuances or use-cases.

@NotTheDr01ds
Copy link
Contributor Author

@amtoine ping as requested

@kubouch
Copy link
Contributor

kubouch commented May 14, 2024

I think this is a great addition, but I would split it a bit. The language guide is meant as a concise reference for the language. There should be only a short mention of what $in is and where it appears. The rest of your text is more of a tutorial style which is better fit for the book, possibly under https://www.nushell.sh/book/variables.html.

Comment on lines 1557 to 1563
#### Implicit `$in`

The preceding example can be simplified even further because `$in |` is implicit at the beginning of a closure. So we can just use the more concise:

```nushell
ls | update name {str upcase}
```
Copy link
Member

@sholderbach sholderbach May 14, 2024

Choose a reason for hiding this comment

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

I think this section is misleading because there is no hidden $in variable in our internal semantics.

Instead part of the custom command/closure semantics is that the first pipeline element of the first pipeline expression will receive the pipeline input to the custom command/closure. (We should make sure that this is both documented in the spec and the general user documentation)

This has the distinct difference that it follows the pipelining semantics instead of the value/variable semantics. Thus it is lazy and carries the pipeline metadata (latter can be considered somewhat of an implementation detail for now)

We may implement optimizations for $in in the "first element of first pipeline"-position but from an educational perspective with the current semantics of Nushell it is much easier to teach $in is a special variable that you can use if needed but the pipelining behavior is expressed with | and the beginning of a custom command/closure block. (Teaching it that way also makes you aware of potential pitfalls, that you can not assume to receive no input in the first pipeline element of a custom command/closure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough - I'll work on fixing that. Agree that the value/variable semantics are different enough (in streaming vs. collection) to make that a bad analogy.

@NotTheDr01ds
Copy link
Contributor Author

I think this is a great addition, but I would split it a bit. The language guide is meant as a concise reference for the language. There should be only a short mention of what $in is and where it appears. The rest of your text is more of a tutorial style which is better fit for the book, possibly under https://www.nushell.sh/book/variables.html.

Thanks! And certainly open to seeing how we can split it out. Regardless, I think The Book needs more discussion of $in than is currently there.

Keep in mind, though, that the README for the Language Guide says the goal is:

to provide a little bit of documentation text for context and then many examples of usage

I know it "looks" like a tutorial at first glance, but it's really just 10 examples of usage of $in, along with the explanation of those examples. In that light, doesn't it pretty much fit the criteria for the "many examples" of the Language Guide?

I also think it's a bit overkill for The Book, which it seems should be the intro/tutorial/digest version, right?

@kubouch
Copy link
Contributor

kubouch commented May 16, 2024

The goal of the reference is to have a concise overview of Nushell's syntax, kind of like https://doc.rust-lang.org/reference. The readme probably should be reworded to reflect that better. The goal of the book is to teach users how to use the language which can include examples. If we don't keep this distinction strict, we'll end up with two books.

I believe you can take most of your text, put it under a new section in https://www.nushell.sh/book/variables.html and in the language reference just add a short enumeration of where $in can appear. It can include examples, but just for showcasing the syntax, it doesn't need to showcase all the possible use cases for $in (like the let day = $in example, doesn't need to be in the language guide). Formulations like "Imagine that ..." are better suited for the tutorial-style book as well.

@NotTheDr01ds
Copy link
Contributor Author

Thanks for the feedback, and apologies for the delay getting back around to this.

  • @kubouch - I've moved it to The Book as requested - I agree this is more "essentials" than "language reference" for the most part. I think it fits better in the "Pipelines" section than "Variables", though.

    I still have some questions on what belongs in each doc, though, but I'll take those up on a separate issue. I plan to do a lot more work around this, and I'd like to make sure it's clear where things go.

  • @sholderbach I've removed the reference to the "implicit $in".

@fdncred
Copy link
Collaborator

fdncred commented May 28, 2024

I think we can move ahead with this. Thanks for taking the time to update based on our feedback. We can continue to tweak as time goes by, but this is 100% better than what we had. Thanks.

@fdncred fdncred merged commit ec5939c into nushell:main May 28, 2024
2 checks passed
@NotTheDr01ds NotTheDr01ds deleted the in-lang-ref branch June 17, 2024 17:36
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.

None yet

4 participants