-
Notifications
You must be signed in to change notification settings - Fork 148
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
Refactor README.md and include Approximate Equality as part of the RealModule. #150
Conversation
Hey @markuswntr Thank for the PR. Big fan of Swift Numerics. Take a look at markuswntr#1 - I've made a few improvement suggestions to the entire README based on tech writing experience - hope you like them and we can bundle the changes in this PR: #150: Summary of suggestions:
- Swift Numerics modules are fine-grained; if you need support for Complex
+ Swift Numerics modules are fine-grained. For example, if you need support for
+ Complex numbers ...
- ... import ComplexModule¹ without pulling in everything else in the library:
+ ... import ComplexModule¹ as a standalone module:
...
- as well.
...
[Example in code blocks]
- // All swift-numerics API is now available
+ // The entire `swift-numerics` API is now available
- Because we intend to make it possible to adopt Swift Numerics modules in the
- standard library at some future point...
+ The Swift Numerics project is intended to be adopted in the standard library
+ in future. Therefore, it...
- Swift Numerics is a standalone library separate from the core Swift project. In
+ Swift Numerics is a standalone library that is separate from the core Swift project. In
Let me know what you think. Cheers! |
Enhance Swift Numerics README.md (technical writing perspective)
Thanks to @8bitmp3, this PR now address not only the approximate equality module but also spelling/grammar - so I have changed the title to reflect the changes. |
README.md
Outdated
```swift | ||
import Complex | ||
// I know I only ever want Complex<Double>, so I shouldn't need the generic parameter. | ||
typealias Complex = Complex.Complex<Double> // doesn't work, because name lookup fails. | ||
// You only ever want Complex<Double>, so you shouldn't need the generic parameter. |
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.
The comment here written by the hypothetical user of Swift Numerics (rather than us, the library authors), so "I" is appropriate.
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.
If we can use the second person ("you" - the reader/user) in docs instead of the first person ("I" and "we"), we should do so, according to Google and Microsoft style guides for doc writing. That's the reason for changing it here.
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 @stephentyrone’s point here is that the comment here is not being written in the voice of the documentation writer, and what’s more essential, should not be expressed in that voice, since it is editorializing on particulars and not providing general advice.
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.
@xwu @stephentyrone Gotcha! Agreed
README.md
Outdated
```swift | ||
import Numerics | ||
|
||
// All swift-numerics API is now available | ||
// The entire `swift-numerics` API is now available |
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.
swift-numerics
doesn't seem to be appropriate formatting here; "swift-numerics" is not code. In prose we usually style the project "Swift Numerics".
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.
Agreed, thanks @stephentyrone
README.md
Outdated
2. [ComplexModule](Sources/ComplexModule/README.md) | ||
|
||
1. About [`RealModule`](Sources/RealModule/README.md) | ||
- [`ApproximateEquality`](Sources/RealModule/ApproximateEquality.swift) |
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.
We should definitely link to detail on ApproximateEquality, but we should link to a markdown document, rather than the implementation. Let's remove this for now and open an issue to track adding one.
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.
Thanks! I removed the link and opened an issue: #153
README.md
Outdated
1. [RealModule](Sources/RealModule/README.md) | ||
2. [ComplexModule](Sources/ComplexModule/README.md) | ||
|
||
1. About [`RealModule`](Sources/RealModule/README.md) |
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 don't think "About" adds any clarity here (it actually confuses it). This is a list of modules, and the module is not "About RealModule" it is "RealModule".
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.
@stephentyrone Sounds good! Maybe we can have the title of the section named "# More on modules" or something along those lines? What each link is about/does can be quite important to increase clarity, better a11y, etc. WDYT?
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 heading is fine as-is. It is a list of modules, not a section “on modules.”
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 am just trying to follow commonly accepted guidelines of tech writing for good docs.
The idea in our case is to help any user - whether they're new to Swift or Swift Numerics - to understand whether they should click on each link (this is part of the user experience/human-computer interface fields). So, in a way, this contributes to inclusiveness and accessibility, which are important to keep in mind when writing doc specs.
A few words with basic info can save readers a trip to an external site that may not want to go to (as per the Google style guide, as I couldn't find any relevant info on Microsoft or Apple's guides).
- For instance, this shows it's a README and I'd probably click on it, since READMEs are mostly common knowledge:
[`RealModule` - README](Sources/RealModule/README.md)
[About `RealModule`](Sources/RealModule/README.md)
- Compare this the original, where it's not too clear whether the user, who hasn't spent as much time with Swift Numerics as us, should click on the link. (Is it an API doc, a
.swift
file, a blog post, or an official Apple reference?)
[`RealModule`](Sources/RealModule/README.md)
Anyway, these are just suggestions. Maybe Apple's tech writers can help here.
Cheers!
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.
Personally, I would prefer not adding hard line wraps to this document. It makes future editing much more finicky, and the diffs much less readable. Since it is not the prevailing style for this project to line-wrap Markdown files, I would suggest leaving that alone.
README.md
Outdated
modules with more specialized dependencies (or dependencies that are not | ||
available on all platforms supported by Swift) belong in a separate package. | ||
|
||
The Swift Numerics project is intended to be adopted in the standard library |
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 is not true in toto; in fact, it is contradicted on its face earlier in the document.
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.
Thanks @xwu !
@markuswntr Can you take a look at this?
The diff:
- Because we intend to make it possible to adopt Swift Numerics modules in the
- standard library at some future point...
+ The Swift Numerics project is intended to be adopted in the standard library
+ in future. Therefore, 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'm actually the project owner, and @xwu is correct, the original text is correct, and this suggested change is not. There will surely be some components of Swift Numerics that never make their way into the standard library.
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.
Sounds good!
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.
Thanks @markuswntr for making the changes and @stephentyrone and @xwu for the feedback!
README.md
Outdated
However, there is also a top-level `Numerics` module that simply re-exports the complete public interface of swift-numerics: | ||
|
||
There is also a top-level `Numerics` module that re-exports the complete public | ||
interface of swift-numerics: |
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.
interface of swift-numerics: | |
interface of Swift Numerics: |
I understand that line wraps aren't too user-friendly @xwu They are part of the docs-as-code approach and help users see "the end of the line", since Markdown rendering can't always auto-wrap depending on where you view the files. This linting style has been adopted by certain well-known open source Google projects (they even include it in doc tests). |
I'm confused by this claim; as @xwu called out, wrapping at 80-cols makes diffs much larger than they should be for minor changes, which makes code review harder and more error-prone. This seems to be contrary to "docs-as-code" (one of the things code should permit is easily seeing how it has changed over time, as well as reviewing changes).
Which renderers can't? Certainly whatever Github uses to render for the web does, and so does Xcode, which are by far the two renderers that will be most commonly used. Are there specific renderers that don't, which we expect to be frequently used with the Swift Numerics documentation (and are there reasons why viewing the docs on the web is not a viable option in those cases?) |
@stephentyrone I understand where you and @xwu come from and the GitHub web UI helps to easily the entire text, while applying soft-wrapping and other user-friendly features. I can revert the wrapping changes back so the diffs are more visible - whatever makes it easier for you to review here 👍
I've reviewed code and docs in VSCode and used various IDEs - from IDEA to Xcode - and I'm aware some folks don't always have the default settings set to "auto-wrap": "on". However, many things in modern IDEs are automated by default these days, including the awesome Xcode. So, for instance, if a contributor forks a repo, opens a Markdown file, and decides to make certain changes, they may see some long lines that make up long paragraphs, such as: For some background, there's a GitLab blog post about the pros and cons of wrapping text, that summarizes this quite well: |
Unwrap text, update Swift Numerics README
README.md
Outdated
|
||
- API that is too specialized to go into the standard library, but which is sufficiently general to be centralized in a single common package. | ||
- API that is under active development toward possible future inclusion in the standard library. | ||
|
||
There is some overlap between these two categories, and API that begins in the first category may migrate to the second as it matures and new uses are discovered. | ||
There is some overlap between these two categories, and an API that begins in the first category may migrate into the second as it matures and more users start using Swift Numerics. |
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.
Originally: "...and new uses are discovered." was 100% good. Let's revert here
Revert "new uses are discovered" in Swift Numerics README
README.md
Outdated
Swift Numerics provides a set of modules that support numerical computing in Swift. | ||
These modules fall broadly into two categories: | ||
|
||
Swift Numerics provides a set of modules that support numerical computing in Swift. These modules fall broadly into two categories: |
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.
While we don't want to wrap, we do want one sentence per line.
Swift Numerics provides a set of modules that support numerical computing in Swift. These modules fall broadly into two categories: | |
Swift Numerics provides a set of modules that support numerical computing in Swift. | |
These modules fall broadly into two categories: |
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.
Agreed. The proposed change looks good - will incorporate, thanks @stephentyrone
README.md
Outdated
There is some overlap between these two categories, and API that begins in the first category may migrate to the second as it matures and new uses are discovered. | ||
There is some overlap between these two categories, and an API that begins in the first category may migrate into the second as it matures and new uses are discovered. | ||
|
||
Swift Numerics modules are fine-grained. For example, if you need support for Complex numbers, you can import ComplexModule¹ as a standalone module: |
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.
Swift Numerics modules are fine-grained. For example, if you need support for Complex numbers, you can import ComplexModule¹ as a standalone module: | |
Swift Numerics modules are fine-grained. | |
For example, if you need support for Complex numbers, you can import ComplexModule¹ as a standalone module: |
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.
Thanks @stephentyrone - moving this sentence to the next paragraph LGTM
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.
To be clear, this is not a new paragraph. New paragraphs only occur in markdown on a double line break. Putting only one sentence per line falls out from docs-as-code practice; a sentence is a code statement.
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.
@stephentyrone Got it!
README.md
Outdated
The current modules assume only the availability of the Swift and C standard libraries and the runtime support provided by compiler-rt. | ||
Future expansion may assume the availability of other standard interfaces such as [BLAS (Basic Linear Algebra Subprograms)](https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms) and [LAPACK (Linear Algebra Package)](https://en.wikipedia.org/wiki/LAPACK), but modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. | ||
|
||
Future expansion may assume the availability of other standard interfaces, such as [BLAS (Basic Linear Algebra Subprograms)](https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms) and [LAPACK (Linear Algebra Package)](https://en.wikipedia.org/wiki/LAPACK). But modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. |
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.
Future expansion may assume the availability of other standard interfaces, such as [BLAS (Basic Linear Algebra Subprograms)](https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms) and [LAPACK (Linear Algebra Package)](https://en.wikipedia.org/wiki/LAPACK). But modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. | |
Future expansion may assume the availability of other standard interfaces, such as [BLAS (Basic Linear Algebra Subprograms)](https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms) and [LAPACK (Linear Algebra Package)](https://en.wikipedia.org/wiki/LAPACK). | |
But modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. |
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.
👍 @stephentyrone Will make this change
README.md
Outdated
Swift Numerics is a standalone library separate from the core Swift project. | ||
In practice, it will act as a staging ground for some APIs that may eventually be incorporated into the Swift Standard Library, and when that happens such changes will be proposed to the Swift Standard Library using the established evolution process of the Swift project. | ||
|
||
Swift Numerics is a standalone library that is separate from the core Swift project. In practice, it will act as a staging ground for some APIs that may eventually be incorporated into the Swift Standard Library. When that happens, such changes will be proposed to the Swift Standard Library using the established evolution process of the Swift project. |
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.
Swift Numerics is a standalone library that is separate from the core Swift project. In practice, it will act as a staging ground for some APIs that may eventually be incorporated into the Swift Standard Library. When that happens, such changes will be proposed to the Swift Standard Library using the established evolution process of the Swift project. | |
Swift Numerics is a standalone library that is separate from the core Swift project, but it will sometimes act as a staging ground for APIs that will later be incorporated into the Swift Standard Library. | |
When that happens, such changes will be proposed to the Swift Standard Library using the established evolution process of the Swift project. |
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.
Agree with the rewording and splitting into separate lines (not paragraphs) 👍 Will make these changes
README.md
Outdated
¹ Swift is currently unable to use the fully-qualified name for types when a type and module have the same name (discussion here: https://forums.swift.org/t/pitch-fully-qualified-name-syntax/28482). | ||
This would prevent users of Swift Numerics who don't need generic types from doing things like: | ||
|
||
¹ Swift is currently unable to use the fully-qualified name for types when a type and module have the same name (discussion here: https://forums.swift.org/t/pitch-fully-qualified-name-syntax/28482). This would prevent users of Swift Numerics who don't need generic types from doing things, such as: |
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.
¹ Swift is currently unable to use the fully-qualified name for types when a type and module have the same name (discussion here: https://forums.swift.org/t/pitch-fully-qualified-name-syntax/28482). This would prevent users of Swift Numerics who don't need generic types from doing things, such as: | |
¹ Swift is currently unable to use the fully-qualified name for types when a type and module have the same name (discussion here: https://forums.swift.org/t/pitch-fully-qualified-name-syntax/28482). | |
This would prevent users of Swift Numerics who don't need generic types from doing things, such as: |
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.
👍👍
README.md
Outdated
New modules have to evaluate this decision carefully, but can err on the side of adding the suffix. | ||
It's expected that most users will simply `import Numerics`, so this isn't an issue for them. | ||
|
||
The `Real` module does not contain a `Real` type, but does contain a `Real` protocol, and users may want to define their own `Real` type (and possibly re-export the `Real` module) - that is why the suffix is also applied there. New modules have to evaluate this decision carefully, but can err on the side of adding the suffix. It's expected that most users will simply `import Numerics`, so this isn't an issue for them. |
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.
The `Real` module does not contain a `Real` type, but does contain a `Real` protocol, and users may want to define their own `Real` type (and possibly re-export the `Real` module) - that is why the suffix is also applied there. New modules have to evaluate this decision carefully, but can err on the side of adding the suffix. It's expected that most users will simply `import Numerics`, so this isn't an issue for them. | |
The `Real` module does not contain a `Real` type, but does contain a `Real` protocol. | |
Users may want to define their own `Real` type (and possibly re-export the `Real` module)--that is why the suffix is also applied there. | |
New modules have to evaluate this decision carefully, but can err on the side of adding the suffix. | |
It's expected that most users will simply `import Numerics`, so this isn't an issue for them. |
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.
👍👍 Thanks @stephentyrone
Update Swift Numerics README to address feedback
README.md
Outdated
¹ Swift is currently unable to use the fully-qualified name for types when a type and module have the same name (discussion here: https://forums.swift.org/t/pitch-fully-qualified-name-syntax/28482). | ||
This would prevent users of Swift Numerics who don't need generic types from doing things like: | ||
This would prevent users of Swift Numerics who don't need generic types from doing things, such as: |
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 would prevent users of Swift Numerics who don't need generic types from doing things, such as: | |
This would prevent users of Swift Numerics who don't need generic types from doing things such as: |
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.
Got it @xwu Thanks!
README.md
Outdated
Future expansion may assume the availability of other standard interfaces such as [BLAS (Basic Linear Algebra Subprograms)](https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms) and [LAPACK (Linear Algebra Package)](https://en.wikipedia.org/wiki/LAPACK), but modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. | ||
|
||
Future expansion may assume the availability of other standard interfaces, such as [BLAS (Basic Linear Algebra Subprograms)](https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms) and [LAPACK (Linear Algebra Package)](https://en.wikipedia.org/wiki/LAPACK). | ||
But modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. |
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.
But modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. | |
, but modules with more specialized dependencies (or dependencies that are not available on all platforms supported by Swift) belong in a separate package. |
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.
Thanks for reviewing @xwu, appreciate it. If you think it's important to have this section as one long sentence, I can definitely make this change!
I noticed that in GitHub's web UI, the whole paragraph takes up three lines. If we merge these sentences, they become a one long thing spanning three lines:
Here's the reasoning behind simplifying a large sentence into two shorter ones (these are guidelines, not rules):
From Google Technical Writing - Short sentences:
"Shorter documentation reads faster than longer documentation."
"Finding the shortest documentation implementation takes time but is ultimately worthwhile. Short sentences communicate more powerfully than long sentences, and short sentences are usually easier to understand than long sentences."
From Microsoft Style Guide - Writing Tips:
"These practices will help localizers and customers.
"Write short, simple sentences. Punctuating a sentence with more than a few commas and end punctuation usually indicates a complex sentence. Consider rewriting it or breaking it into multiple sentences."
https://developers.google.com/tech-writing/one/short-sentences
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 is one thought, not two, so it needs to be one sentence.
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.
Ok @xwu! Will make the changes
Update Swift Numerics README to address feedback
Hi @markuswntr, can you update this to point against main instead of master? Thanks. |
Hi @stephentyrone, updated base branch to main. Thanks! |
Approximate Equality has recently been merged into master. This PR updates the README.md to reflect the changes.
While it is part of the
RealModule
it really is an extension onNumeric
, so I decided to list it below theRealModule
but separately.