-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Update: (root) README #2010
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
Update: (root) README #2010
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.
While a quick glance at the resulting README looks pleasant, I am quite concerned about the form of this PR.
Take for example the commit message:
Update: (root) README
That's it. I would be much more convinced of these changes if the commit message followed the guidance in https://github.blog/2022-06-30-write-better-commits-build-better-projects/ to improve it, in particular with a strong focus on this part:
What you’re doing | Why you’re doing it | |
---|---|---|
High-level (strategic) | Intent (what does this accomplish?) | Context (why does the code do what it does now?) |
Low-level (tactical) | Implementation (what did you do to accomplish your goal?) | Justification (why is this change being made?) |
And then there are many, many, many changes, all squashed into a single commit, making it very, very easy to miss anything undesired.
Simple example: the ```console
construct is supposed to show a shell session, including prompts. Yet all of those $
prompts are removed. Hrmpf.
Other example, some of the more in-depth information is simply removed. Maybe an improvable idea?
Instead, a large set of changes should be structured into individual commits, each with a commit message that makes a good case for the introduced changes.
One such commit could be to add a table of contents. That should probably even be one of the first commits of this PR, if not the first.
Then, if you still insist on removing those $
prompts from the shell sessions, that would need to be its own commit (if only to make it easier to drop that part and not having to drop all other changes at the same time).
Don't get me wrong, I like a lot what I am seeing in the result. It's just that the current form of the PR makes it really, really hard to improve it. And improving it still needs, from my perspective at least.
|
||
> [!NOTE] |
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 pretty concerned about removing an important visual marker like this.
I am not saying that you have to reinstate it if the new look gets too broken up by it, but I am saying that simply stating that the site was rewritten is a tad too little to convince me that all of these broad changes that are smooshed into a single commit are actually good.
|
||
Supported browsers include `firefox`, `chromium`, `webkit`, `chrome`. You can also simply download all of them using `npx playwright install` but please first note that they all weigh >100MB, so you might want to refrain from doing that. Side note: In GitHub Actions' hosted runners, Chrome comes pre-installed, and you might be able to use your own Chrome installation, too, if you have 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.
Hmm. I'm not a fan of dropping this important information on the floor.
I'd just like to state that I stopped at only two comments because I anticipate that it will be much easier to review and improve this PR once the changes are broken up into a proper set of commits, not because there is nothing more to comment on. |
Hey @dscho , I understand your concerns, I'll redo this PR in the manner you have suggested. |
@RafaelJohn9 thank you. BTW if I were you, I'd use the technique described in https://git-scm.com/docs/git-rebase#_splitting_commits to avoid redoing the PR but instead simply splitting all the existing changes into appropriate commits. I find that |
thanks 👍 , everyday is a new learning day 😅 |
Totally! |
Move badges below the title to improve visual hierarchy. Reword the opening paragraph to make the purpose of the site clearer for first-time visitors.
Replace initial local development note with a comprehensive Table of Contents Organize sections for easy navigation: Getting Started, Local Development, Testing, Content Updates, Contributing, etc.
Introduce "Cloning the Repository" guide Recommend using scalar for efficient sparse cloning
Make commands easier to copy and paste without errors
Add "Alternative: Manual Sparse Clone" section for users without scalar Provide clean, prompt-free shell commands to improve copy-pasting Enhance usability of the cloning instructions
Reformat note for better readability and emphasis Clarify instructions for users with a full clone to focus on subsets Improve tone and consistency in documentation style
Provide brief explanations for each relevant directory Improve clarity for developers navigating the project layout
…uctions Add a new "Local Development" section with subsections for prerequisites and serving the site Specify required tools: Hugo (extended v0.128.0+), Node.js, and Windows WSL recommendation Improve instructions for verifying Hugo version and serving the site locally Clarify note about pre-rendered directories and GitHub workflows
Reword instructions to use Hugo’s built-in server. Explain the need to disable “ugly URLs” for compatibility with GitHub Pages Add a clear note defining “ugly URLs” and why serve-public.js emulates GitHub Pages behavior
…atting Introduce instructions to build the site with search enabled using Hugo and Pagefind Remove $ prompt symbols from shell commands to improve copy-pasting experience Improve clarity and usability of local testing documentation
Rephrase to highlight using Pagefind’s built-in server. Remove $ from shell commands for easier copy-pasting Add a clear note about slower performance and disabled live reload when using Pagefind
Rewrite introduction to describe Playwright test suite and location clearly Add “Running Tests” subsection with step-by-step instructions Remove $ prompt symbols from shell commands to simplify copy-pasting Improve overall structure and readability of the testing setup documentation
Reorganize the Playwright testing section to improve clarity and usability. Adds headings, formatting, and notes on browser download sizes. Also explains how to run tests against local changes and filter tests with regex.
Add clear headings under 'Content Updates' for installing Ruby dependencies and generating manual pages. Reorganize steps to separate local and remote build instructions, and format code blocks consistently for better readability.
…teps Structured manual page instructions into local and GitHub-based workflows. Added steps for building localized manual pages using git-html-l10n. Improved formatting and clarity for version-specific builds and prerequisites.
Renamed "Update the ProGit book" section to "ProGit Book" for clarity. Added step-by-step formatting for prerequisites and script usage. Clarified how to fetch and update book content from local repositories. Updated download data instructions for improved readability.
Reorganized steps into clearly numbered list. Added cloning instructions and clarified directory structure. Documented alternative GitHub-based update method using API tokens. Explained how to build the book for all available languages.
Encouraged contributions with a more welcoming tone. Recommended using scalar and sparse-checkout for efficiency. Added guidance on making pull requests and opening issues for features.
Added a new section with step-by-step guidance on contributing GUI client entries. Clarified metadata fields required in the .md file. Provided image naming and formatting requirements. Ensured platform and ordering fields are clearly explained.
Promoted "Hugo", "Pagefind", "Lychee", and "Playwright" to top-level headings. Reorganized and standardized the formatting of resource links. Improved clarity and readability by aligning structure with other sections.
Promoted license section to a top-level heading. Consolidated license descriptions for source code and graphical assets. Enhanced readability and link presentation.
39c6486
to
863f29f
Compare
Hey @dscho, what's your opinion on this PR ? |
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 there is a lot of good stuff in this PR, there is at least an equal amount of unfortunate stuff that I have a hard time to bring myself to like. It was quite strenuous to try to make sense of those diffs in the commits, as it seems to me that the overall huge, honking, unreviewable diff was merely split at certain hunks, without regard whether the retained hunks actually reflect what their commit messages tried to convey. For example, removing a hint to run one script with a hint to write another script is wrong, and the underlying problem is that the removed hint should have been in a totally different commit (namely the one that rephrased that part of the README).
In its current form, I am not willing to merge this PR.
Do note, though, that I am not the maintainer of this site. There are other people who could merge this PR if you want to convince them (a strategy I would totally condone).
|
||
#### 1. Recommended Approach: Using Scalar | ||
|
||
We recommend using [`scalar`](https://git-scm.com/docs/scalar) for an efficient and focused clone. This allows you to work only on the parts of the repository relevant to your interests. |
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 still concerned about the removal of that > [!NOTE]
marker. It is a rather important thing that this sentence talks about, which would likely be missed otherwise.
```console | ||
$ scalar clone https://github.com/git/git-scm.com | ||
$ cd git-scm.com/src | ||
$ git sparse-checkout set layouts content static assets hugo.yml data script | ||
scalar clone https://github.com/git/git-scm.com | ||
cd git-scm.com/src | ||
git sparse-checkout set layouts content static assets hugo.yml data script |
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 you remove the $
, then the console
markup is wrong, it should be sh
instead.
@@ -53,8 +53,8 @@ git sparse-checkout set layouts content static assets hugo.yml data script | |||
git reset --hard | |||
``` | |||
|
|||
> [!NOTE] | |||
> If you _already_ have a full clone and wish to accelerate development by focusing only on a small subset of the pages, you may want to run the `git sparse-checkout set [...]` command mentioned above. | |||
> **Note:** |
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.
Please compare the visual of > [!NOTE]
vs **Note:**
. I find the former vastly better and would highly recommend to keep it.
- To add new GUIs: | ||
- data/ |
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 you may find it clearer, the original intention of this section was to answer the question "Which directories do I need in my sparse checkout?". That's kind of gone, the reader now has to cobble together what they need from that admittedly neat-looking (but less helpful) bullet list.
- To work on the GitHub workflows that perform the automated, scheduled pre-rendering: | ||
- .github/ | ||
## Local Development |
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.
|
||
Since Playwright uses headless versions of popular web browsers, you most likely need to install at least one of them, e.g. via: |
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 another important piece of information that seems to be culled in the name of clarity. I do disagree that this is a good idea.
This will populate the manual pages for all Git versions. You can also populate them only for a specific Git version (faster): | ||
## Content Updates | ||
|
||
```console | ||
$ version=v2.23.0 | ||
$ REBUILD_DOC=$version ruby ./script/update-docs.rb /path/to/git/.git en | ||
``` | ||
### Manual Pages | ||
|
||
Or you can populate the man pages from GitHub (much slower) like this: | ||
1. Install Ruby prerequisites: | ||
|
||
```console | ||
$ export GITHUB_API_TOKEN=github_personal_auth_token | ||
$ REBUILD_DOC=$version ruby ./script/update-docs.rb remote en # specific version | ||
``` | ||
```console | ||
bundler install | ||
``` |
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.
Is it possible that you simply split the diff at the boundaries of the new section? If you carefully look at the removed lines and what was added in their place, the diff is non-sensical. I would have expected original text to be replaced by improved text, but basically talking about the exact same thing. That's distinctly not the case here.
```console | ||
$ ruby ./script/update-download-data.rb | ||
``` | ||
For localized man pages, use a local clone of [git-html-l10n](https://github.com/jnavila/git-html-l10n): | ||
|
||
```console | ||
ruby ./script/update-docs.rb /path/to/git-html-l10n/.git l10n | ||
REBUILD_DOC=$version ruby ./script/update-docs.rb /path/to/git-html-l10n/.git l10n | ||
``` |
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.
Why is update-download-data
replaced with update-docs
? This diff makes no sense, it is highly unclear what was improved, and in what way, and whether it is an improvement altogether. For a PR that wants to improve the clarity of the document, the PR itself lacks quite a bit of clarity.
## Update the ProGit book | ||
### Downloads Data | ||
|
||
First, you will have to get the necessary prerequisites: | ||
Update the downloads data for the site: | ||
|
||
```console | ||
$ bundler install | ||
ruby ./script/update-download-data.rb |
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 diff makes no sense.
All graphical assets are licensed under the | ||
[Creative Commons Attribution 3.0 Unported License](https://creativecommons.org/licenses/by/3.0/). | ||
The source code for this site is licensed under the MIT license (see `MIT-LICENSE.txt`). | ||
All graphical assets are licensed under the [Creative Commons Attribution 3.0 Unported License](https://creativecommons.org/licenses/by/3.0/). |
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.
Removing that line break makes the diff less clear.
Hey @dscho , Thank you for your detailed review , I apologize if this PR seems like a mess and does not satisfy your standards. if its okay, I would like to close this for now and create issues while submitting PRs. That would make it easier to review and make our discussion not so over the place and rather be concentrated into specific points. |
Totally okay with me. But as I said, I am not the maintainer of this site (even if it seems that the onus of reviewing and merging PRs somehow falls mostly on me, I guess I wasn't up in the tree at the count of three). |
hey @dscho , you are who we work with :) 👍 |
Changes
Context
I’ve updated the
README.md
to improve the overall clarity and structure. The goal of these changes is to make it easier for new contributors and developers to understand how to set up the project locally, run tests, and begin contributing. I’ve tried to keep the original intent and flow intact, while rewording or reorganizing certain sections for better readability and guidance.Thank you for your time and for reviewing the changes!