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

Fixup and improve book using_modules.md #1608

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

Conversation

Kissaki
Copy link
Contributor

@Kissaki Kissaki commented Nov 1, 2024

See individual commits for reasoning.

@Kissaki
Copy link
Contributor Author

Kissaki commented Nov 1, 2024

Unrelated CI issue has suggested fix in #1606

The relative example follows afterwards. This block only contains docs on absolute paths.
`$env.NU_LIB_PATH` does not exist

In the block that follows the correct name is being used.
The blocks demonstrate different things, but it's confusing to have different code - mixing both behavior changes through context and unnecessary code changes.
Increases readability and clarity.

Using 'simply' can indicate that actions are simple. But adding it without a need for differentiation or qualification, they increase sentence complexity, making it harder to read and parse. The statements remain just as valid without them.
The concern before this one already has split sections.

Rather than a block with two mixed concerns, and a need for "or"  text in a mixed-concern example attempting to demonstrate two things at once,
having two blocks makes it much more obvious, distinct, and reference-able (when the reader is only interested in one aspect).

It also simplifies the NU_LIB_DIRS concern a lot. Instead of a long block headline with a lot of mixed information, we do the same as the other concern earlier in the document:
note the information in an important note block.
@fdncred
Copy link
Collaborator

fdncred commented Nov 6, 2024

@NotTheDr01ds has been making changes to modules too. I'd like to get his take on this before we land it.

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.

Thanks for the updates! I've submitted feedback on most of them. @fdncred - Would appreciate your opinion on my feedback as well if you get a chance. Thanks!

book/modules/using_modules.md Outdated Show resolved Hide resolved
When importing a module via a relative path, Nushell first searches from the current directory. If a matching module is not found at that location, Nushell then searches each directory in the `$env.NU_LIB_DIRS` list.

This allows you to install modules to a location that is easily accessible via a relative path regardless of the current directory.
:::

- An absolute or relative path to a Nushell module file. As above, Nushell will search the `$env.NU_LIB_DIRS` for a matching relative path.
- An absolute path to a Nushell module file:
Copy link
Contributor

Choose a reason for hiding this comment

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

I did have "absolute" and "relative" split out originally, but I combined them since there is so much duplicated wording in the "split" version. While I think it's better combined, I can certainly see reasons for splitting relative/absolute into separate bullets as well. However, if we split them, then (a) We should move "relative" above "absolute" since it is the most common form. And (b) I would recommend not duplicating the entire "Important" block - Just reference it the second time, rather than repeating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the fixup, the first two was split and the second was combined. I think that asymmetry should be solved, one way or another.

I agree with order should be relative before absolute

I prefer self-sufficient explanations, without pointing to earlier sections. I would prefer duplication over pointing elsewhere, especially if it's not a anchor jump link

Given the noteworthy concerns, maybe it makes more sense to combine the two absolute and the two relative blocks? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Before the fixup, the first two was split and the second was combined
Given the noteworthy concerns, maybe it makes more sense to combine the two absolute and the two relative blocks?

Ah - I think I had a good reason for it, but I also agree with you on the symmetry. I'll take another look at it, but it might be a few hours.

Copy link
Contributor Author

@Kissaki Kissaki Nov 18, 2024

Choose a reason for hiding this comment

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

I'm fine with leaving it as it was. Personally, I like reference documentation. But this is a "book". So I'm reminding myself that the form is more of a read-in-order thing where reading order and block-cross-references can be expected. Unfortunately, there's no reference then though. :)

The list is also quite long. So reducing the number of items is a good idea if not necessary.

@@ -100,7 +114,7 @@ The path to the module can be:

:::

- Less commonly, the name of a module already created with the [`module`](/commands/docs/module.md) command. While it is possible to use this command to create a module at the commandline, this isn't common or useful. Instead, this form is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).
- Less commonly, the name of a module is created with the [`module`](/commands/docs/module.md) command. While it is possible to use this command to create a module at the commandline, this isn't common or useful. Instead, this form is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).
Copy link
Contributor

Choose a reason for hiding this comment

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

This results in a grammatically incorrect sentence flow. Keep in mind that the sentence leading into this list is "The path to a module can be", so the sentence has now gone from:

The path to a module can be ... Less commonly, the name of a module already created with the module command.

to:

The path to a module can be ... the name of a module is created with the module command.

However, I agree the original can be improved a bit. How about:

Suggested change
- Less commonly, the name of a module is created with the [`module`](/commands/docs/module.md) command. While it is possible to use this command to create a module at the commandline, this isn't common or useful. Instead, this form is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).
- Less commonly, the name of a module that was previously created with the [`module`](/commands/docs/module.md) command. While it is possible to use this command to create a module at the commandline, this isn't common or useful. Instead, this form is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, you mean to say the list intro "The path to the module can be:" is continued in this sentence?

That's 74 code lines, or four list items each with code block and two warning notice blocks above here. I don't think that can still be considered "sentence flow". A list that is not a list of one-liners should have self-sufficient bullet points / bullet point sections.

I certainly see why I didn't realize what the sentence was referring to / that there's a pre-text to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see how the other points use that sentence flow so it wouldn't make sense to do that differently here. But I don't think how it was was clear with how distant the whole thing is.

Copy link
Contributor Author

@Kissaki Kissaki Nov 7, 2024

Choose a reason for hiding this comment

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

I think your suggestion clarifies it quite well. But maybe now I'm just aware of the context. Dunno. It seems like it should have the same issue for me, but it doesn't when I read it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lol - I get it. Yes, "sentence flow" may not be as accurate a description as "consistent bullet styles" (which I'm also okay sometimes deviating from when necessary). Feel free to commit the suggestion if you'd like. Or spend the night on it and see how it looks in the morning ;-).

Copy link
Contributor Author

@Kissaki Kissaki Nov 18, 2024

Choose a reason for hiding this comment

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

I think the list item should begin in the same form as the others.

Suggested change
- Less commonly, the name of a module is created with the [`module`](/commands/docs/module.md) command. While it is possible to use this command to create a module at the commandline, this isn't common or useful. Instead, this form is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).
- An already defined module: The [`module` command](/commands/docs/module.md) can define a module. It is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules). It is not usually used to define "normal" modules.

Dunno about the last sentence. "normal" module? root level? "main modules"? "Not usually used", "not commonly used", "we recommend against using it to …"?

Suggested change
- Less commonly, the name of a module is created with the [`module`](/commands/docs/module.md) command. While it is possible to use this command to create a module at the commandline, this isn't common or useful. Instead, this form is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).
- An already defined module: The [`module` command](/commands/docs/module.md) can define a module. We recommend against using it to define modules. It is primarily used by module authors to define a submodule. See [Creating Modules - Submodules](./creating_modules.md#submodules).

What do you think?

book/modules/using_modules.md Outdated Show resolved Hide resolved
book/modules/using_modules.md Outdated Show resolved Hide resolved
book/modules/using_modules.md Outdated Show resolved Hide resolved
@NotTheDr01ds NotTheDr01ds requested review from NotTheDr01ds and removed request for NotTheDr01ds November 7, 2024 02:57
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