[#6600] docs: Refactor the how-to-build docs #6659
Conversation
|
/assign @jerryshao |
There was a problem hiding this comment.
@justinmclean Thanks for the review.
Most suggestions accepted and issues are fixed accordingly.
There is one suggestion I don't agree with you, please help double check?
| If you want to contribute to this open-source project, please fork the project on GitHub first. After forking, clone the forked project to your local environment, make your changes, and submit a pull request (PR). | ||
| :::note | ||
| Depending on how you deploy Gravitino, there may be contain known security vulnerabilities | ||
| in other software used in conjunction with Gravitino. |
There was a problem hiding this comment.
I missed this in my first review, and this shows why this is an issue. Breaking this sentence over two lines hides an error. Please keep the orignal text "Depending on how you deploy Gravitino, other software used in conjunction with Gravitino may contain known security vulnerabilities."
There was a problem hiding this comment.
Okay. If you hate breaking long lines, try figure out a way breaking this sentences to two.
There was a problem hiding this comment.
I don't think there is any need to do that either. BTW this was added as due to an ASF requirement.
docs/how-to-build.md
Outdated
| :::note | ||
| The Gravitino libraries built are Java 8 compatible and verified under Java 8, 11, and 17. | ||
| You can run the Gravitino server using JRE 8, 11, or 17, | ||
| regardless which JDK version was used to build the project. |
There was a problem hiding this comment.
Again splitting this over two lines hides an issue. it should be "regardless of which"
There was a problem hiding this comment.
I can fix this.
But, again, this issue is not caused by line breaking.
There was a problem hiding this comment.
In other words, the error doesn't become more evident when we merge these two lines into a single line.
On the contrary, long lines make it hard for catching errors like this.
There was a problem hiding this comment.
If the two lines are together in one sentence, and you use a grammar checker, you see the error right away as it is underlined in red. With the line break, you may not see the error as a grammar check will not see the error, so yes the added line breaks did contribute to this error being missed on my first review.
There was a problem hiding this comment.
It is your grammar checker's problem. It requires a sentence to be written in a single line. If that is the case, forget the tool. Trust your eyes and your brain.
There was a problem hiding this comment.
We are all fallible. Mistakes happen, but tooling can easily catch many of these errors if you let it. You have introduced several of these issues in this and other PRs, but I assume you trusted your eyes and brain to avoid making those mistakes. Breaking up sentences increases the risk of errors like this being introduced, and manual reviews will not catch all of these issues.
What changes were proposed in this pull request?
This PR refactors the how-to-build guide.
Why are the changes needed?
The current how-to guide has some duplicated contents. For example,
Related: #6600
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
N/A