-
Notifications
You must be signed in to change notification settings - Fork 127
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
Eliminate 150 Vale Warnings #1276
base: main
Are you sure you want to change the base?
Conversation
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 the PR, there was recently a big change on the documentation code. Would you mind rebasing?
Reviewable status: 0 of 2 LGTMs obtained, and 0 of 13 files reviewed, and pending CI: pre-commit-checks
cda7297
to
0e03113
Compare
@allada, I rebased and resolved the conflicts (there were 3 conflicting links and I accepted the new fixed links). I'm surprised that some of the CI tests are failing. I only edited md and mdx files, which shouldn't break any code. I ran I saw some error messages mention "Unsupported OS version"; I'm using MacOS 14.6.1 Sonoma on Apple silicon. Thoughts? |
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.
Wow, nice work!
Regarding the execution
term, it's a but suboptimal, but I think we need to keep it since the RBE is the remote build execution protocol.
Reviewed 8 of 13 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: docker-compose-compiles-nativelink (22.04), integration-tests (22.04), and 21 discussions need to be resolved
-- commits
line 4 at r2:
nit: Add this to link the commit to the vale issue:
Towards #1086
README.md
line 19 at r2 (raw file):
[![License](https://img.shields.io/badge/License-Apache_2.0-blue.svg)](https://opensource.org/licenses/Apache-2.0) ## What's NativeLink
nit: This looks a bit off. Maybe change it to ## About NativeLink
README.md
line 21 at r2 (raw file):
## What's NativeLink NativeLink is an efficient, high-performance build cache and remote operation system that accelerates software compilation and testing while reducing infrastructure costs. It optimizes build processes for projects of all sizes by intelligently caching build artifacts and distributing tasks across several machines.
I think it's fine to use the term execution
here, since RBE
stands for Remote Build Execution
and NativeLink is essentially an implementation of that protocol.
To allow the term you can add [Ee]xecution
to this file: https://github.com/TraceMachina/nativelink/blob/main/.github/styles/config/vocabularies/TraceMachina/accept.txt
Code quote:
operation
README.md
line 35 at r2 (raw file):
- Significantly reduces build times, especially for incremental changes 2. **Efficient Remote Execution**:
As above.
README.md
line 45 at r2 (raw file):
## 🚀 Quickstart To start, you can deploy NativeLink as a Docker image (as shown below) or by using a cloud-hosted solution provided by the NativeLink team - [NativeLink Cloud](https://app.nativelink.com). It's **FREE** for individuals, open-source projects, and cloud production environments, with support for unlimited team members.
Suggestion:
the
README.md
line 45 at r2 (raw file):
## 🚀 Quickstart To start, you can deploy NativeLink as a Docker image (as shown below) or by using a cloud-hosted solution provided by the NativeLink team - [NativeLink Cloud](https://app.nativelink.com). It's **FREE** for individuals, open-source projects, and cloud production environments, with support for unlimited team members.
Suggestion:
container
SECURITY.md
line 35 at r2 (raw file):
Nix derivation hash tag the images. The latest pushed image corresponds to the `main` branch. GitHub action producing an image signs that image. Note that the [OCI workflow](https://github.com/TraceMachina/nativelink/actions/workflows/image.yaml) might take some time to publish the latest image.
This change looks wrong. (It's also a bit risky to change since it's part of the security related information. Feel free to omit it from this PR)
deployment-examples/chromium/README.md
line 33 at r2 (raw file):
> the stack with `pulumi stack` and `pulumi destroy`. Next, start some standard deployments. This step includes building and preparing the remote containers for use in the cluster.:
nit: line break at 80 characters
docs/src/content/docs/deployment-examples/on-prem-overview.mdx
line 18 at r2 (raw file):
Each example leverages some latest, cutting-edge Kubernetes configurations, but with a simplified architecture that prioritizes
Suggestion:
Each example leverages Kubernetes with a simplified architecture that prioritizes
docs/src/content/docs/faq/remote-execution.mdx
line 7 at r2 (raw file):
--- Remote processing is a powerful feature that allows you to distribute build
Suggestion:
execution
docs/src/content/docs/faq/remote-execution.mdx
line 14 at r2 (raw file):
- **Faster build and test operation**: By distributing actions across different nodes, you can carry out builds and tests in parallel, reducing the time required.
Suggestion:
run
docs/src/content/docs/faq/remote-execution.mdx
line 15 at r2 (raw file):
- **Faster build and test operation**: By distributing actions across different nodes, you can carry out builds and tests in parallel, reducing the time required. - **Consistent processing environment**: Remote processing ensures that all members of
Suggestion:
execution
docs/src/content/docs/faq/remote-execution.mdx
line 17 at r2 (raw file):
- **Consistent processing environment**: Remote processing ensures that all members of a development team are working in the same environment, reducing the "it works on this machine" problem.
This is a figure of speech, so it should remain it works on my machine
.
Code quote:
it works on this machine
docs/src/content/docs/faq/remote-execution.mdx
line 22 at r2 (raw file):
For more information on how to get started with remote execution in NativeLink, please refer to [NativeLink On-Prem Guide](https://docs.nativelink.com/introduction/on-prem).
Suggestion:
please refer to the [NativeLink On-Prem Guide](https://docs.nativelink.com/introduction/on-prem).
docs/src/content/docs/faq/rust.mdx
line 12 at r2 (raw file):
all the languages that are fast and non garbage collected, Rust stands out as the sole language that provides the necessary guarantees for writing asynchronous code for several distributed systems that communicate over GRPC.
docs/src/content/docs/introduction/on-prem.mdx
line 13 at r2 (raw file):
or specific infrastructure setups. To provide this functionality, NativeLink is seamlessly
Suggestion:
flexibility
docs/src/content/docs/introduction/on-prem.mdx
line 17 at r2 (raw file):
hard to ensure that the process of setting up NativeLink on your own servers is as straightforward as possible and provides comprehensive documentation to guide you through the process.
Let's remove this, it's unnecessary.
Suggestion:
deployable in an on-premises environment.
docs/src/content/docs/introduction/on-prem.mdx
line 20 at r2 (raw file):
## Making your First Deployment To get started with running NativeLink on-premises, it's recommended to take a
Suggestion:
consider taking
local-remote-execution/README.md
line 5 at r2 (raw file):
NativeLink's Local Remote Execution is a framework to build, distribute, and iterate on custom toolchain setups that are transparent, fully hermetic, and reproducible across machines of the same system architecture, while a saving a lot of time.
local-remote-execution/README.md
line 17 at r2 (raw file):
> [!NOTE] > At the moment LRE works just on `x86_64-linux`.
Suggestion:
doesn't work on systems other than `x86_64-linux`.
local-remote-execution/README.md
line 343 at r2 (raw file):
## 📐 Architecture The original C++ and Java toolchain containers are virtually never instantiated.
Suggestion:
never
@aaronmondal sorry for my late reply, and thanks for going through all of my changes. I'll incorporate the changes you recommended shortly. Alongside [Ee]xecution, I'll also add [Ee]xecute to the list of accepted words and I'll revert my previous execution -> operation/processing changes. Quick question about "works on my machine". I'm aware it's a common figure of speech. I changed it as Vale would warn against using "my". One way to bypass this could be to add this phrase to the list of accepted words; I'll have to test if phrases (multiple words = multiple spaces) get parsed as expected. Alternatively, we can ignore this warning for now. What do you think would be the most sustainable approach? |
Towards TraceMachina#1086. This commit only edits documentation material. The goal was to eliminate a significant chunk of vale warnings. I was able to resolve 150 warnings, which is about 35%.
0e03113
to
b539bd5
Compare
@aaronmondal your suggestions have been incorporated. Have a look! |
Description
This PR only edits documentation material. The goal was to eliminate a significant chunk of vale warnings. I resolved 150 warnings from 13 files, constituting about 35% of total warnings.
I had a question about one particular warning. Whenever "execute" or "execution" was present in the text, vale would throw
Be careful with 'Execution', it’s profane in some cases. alex.ProfanityUnlikely
. In the beginning, I replaced "execution" with "processing" or "operation". But then I started coming across proper nouns such as Remote Execution Protocol and Local Remote Execution (LRE). Unsure about removing "execution" from these, I purposely ignored these warnings. Let me know if y'all would like me to revert my earlier swapping of execution with process/operation or leave things be as they are currently.Fixes # 1150
Type of change
How Has This Been Tested?
I went to the root directory and ran
cargo test --all
, which conducted 316 tests and my branch passed all of them. Since I was solely editing documentation, I wasn't expecting any tests to fail ...This change is