Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
Fixup and improve book using_modules.md #1608
Changes from 6 commits
bf790aa
9eea190
eec9199
f790b3f
41f99b4
1abe662
e5de007
9068c24
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
to:
However, I agree the original can be improved a bit. How about:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;-).
There was a problem hiding this comment.
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.
Dunno about the last sentence. "normal" module? root level? "main modules"? "Not usually used", "not commonly used", "we recommend against using it to …"?
What do you think?