docs: Textual improvements in build docs #14583

merland commented at 8:54 AM on October 26, 2018: contributor

While reading the build docs, I found some opportunities for textual improvements (Force of habit, I used to work as a technical writer...)

Added a few missing words, should be uncontroversial.
Changed/added some punctuation, for better flow and readability.
Fixed one Markdown issue, where two list item headings rendered without a line break. (See image) This one needs to be verified after a build, I don't have a proper build environment yet.

fanquake added the label Docs on Oct 26, 2018

promag commented at 9:14 AM on October 26, 2018: member

ACK, these look like reasonable changes, just squash commits. Also, please go easy on PR's like this, like wait until there's a reasonable amount of improvements before creating the pull request.

in doc/build-unix.md:197 in 6202fc4b32 outdated

 193 | @@ -194,7 +194,7 @@ Hardening Flags:
 194 |  
 195 |  Hardening enables the following features:
 196 |  
 197 | -* Position Independent Executable
 198 | +* _Position Independent Executable_

promag commented at 9:15 AM on October 26, 2018:

Remove trailing whitespaces.

merland commented at 9:31 AM on October 26, 2018:

Well, that is the main change, double-space causes a linebreak: https://markdown-guide.readthedocs.io/en/latest/basics.html#line-return

Is there a preferred way of doing linebreaks in this context?

promag commented at 9:46 AM on October 26, 2018:

Either change formatting, update the failing test/lint/lint-whitespace.sh or leave as it is.

merland commented at 9:57 AM on October 26, 2018:

Ah, didn't notice the failed check, sorry. I'll figure something out.

merland commented at 9:42 AM on October 26, 2018: contributor

@promag Thanks for reviewing. Could you please elaborate on why we should go easy on PRs like this? Where can I read more about this rationale? I'm trying to learn how to best contribute.

Small PRs are nice since the conflict risk is lower, but of course they increase the administration overhead. What is the best balance?

I left it un-squashed so that it would be easier to comment/discuss the individual commits. Was planning to squash right before merge.

promag commented at 9:50 AM on October 26, 2018: member

What is the best balance?

I'd say just try to not have more than 1 open PR with these improvements and wait a couple of days /weeks before opening a new one. There is for sure room for a lot of textual improvements and we don't want that to be the focus of the maintainers. Please read https://github.com/bitcoin/bitcoin/blob/master/.github/PULL_REQUEST_TEMPLATE.md.

merland commented at 10:18 AM on October 26, 2018: contributor

@promag I have read the PR template before, but it does not mention small textual improvement PRs. Your suggestion of having only one of this kind of PRs open at time sounds reasonable, but it seems that the governance structure of the project puts a break on "trivial" changes being submitted and accepted. This is a pity, I think.
True excellence can only be achieved if we fix all broken windows, even the tiny ones.

merland force-pushed on Oct 26, 2018

practicalswift commented at 12:39 PM on October 26, 2018: contributor

@merland

This is my perspective:

I'm one of the reviewers who will review your documentation changes.

I got really happy when reading your PR: having a former technical writer looking over and improving our documentation is a great opportunity. Excellent news!

Don't hesitate to ping me if you ever need a documentation PR reviewed.

In the highly unlikely event that you make "too many" high quality documentation improvements we'll find a way to solve that – perhaps by trying to expand the pool of documentation reviewers.

(This might be highly controversial but personally I'm not convinced that PR count is a good proxy for review burden. Personally I feel that review burden is largely a function of LOC touched. Documentation changes are typically tiny in terms of LOC touched.)

Thanks for the contribution! I'm really happy to have you on board :-)

ACK 2b3e0a18fd193b64d01904cf1f6588947b9a41a6 modulo squash

True excellence can only be achieved if we fix all broken windows, even the tiny ones.

Agreed!

in doc/build-osx.md:23 in 2b3e0a18fd outdated

  19 | @@ -20,23 +20,23 @@ Dependencies
  20 |  
  21 |  See [dependencies.md](dependencies.md) for a complete overview.
  22 |  
  23 | -If you want to build the disk image with `make deploy` (.dmg / optional), you need RSVG
  24 | +If you want to build the disk image with `make deploy` (.dmg / optional), you need RSVG.

hebasto commented at 1:55 PM on October 26, 2018:

If you want to build the disk image with make deploy (.dmg / optional), you need RSVG:

merland commented at 7:21 AM on October 27, 2018:

Agreed, this is better. Will fix.

in doc/build-osx.md:39 in 2b3e0a18fd outdated

  37 |  ```
  38 |  
  39 |  from the root of the repository.
  40 |  
  41 | -**Note**: You only need Berkeley DB if the wallet is enabled (see the section *Disable-Wallet mode* below).
  42 | +**Note**: You only need Berkeley DB if the wallet is enabled (see the section *Disable-wallet mode* below).

hebasto commented at 1:59 PM on October 26, 2018:

Note: You only need Berkeley DB if the wallet is enabled (see the section Disable-wallet mode below).

merland commented at 7:23 AM on October 27, 2018:

Sorry, what is the change you are suggesting?

hebasto commented at 7:28 AM on October 27, 2018:

to make section name as a link:

**Note**: You only need Berkeley DB if the wallet is enabled (see the section [*Disable-wallet mode*](/doc/build-osx.md#disable-wallet-mode) below).

hebasto commented at 2:20 PM on October 26, 2018: member

ACK 6202fc4 with some nits:

could references to own sections (e.g., "Disable-wallet mode" section) be clickable and formatted consistently;
some more lines need : at the end;
could be better to replace "Clone the Bitcoin Core source code and cd into bitcoin" with just "Clone the Bitcoin Core source code:"

merland commented at 2:23 PM on October 26, 2018: contributor

@practicalswift Thanks a lot for taking the time to write this. Your comment reflects the atmosphere I was expecting and hoping for when I started to try to contribute to the project. And I also agree about the review burden. Since many trivial changes are by nature independent of each other, they could be done in separate PRs most of the time. I think. :)

hebasto commented at 4:24 PM on October 26, 2018: member

It is recommended to use Berkeley DB 4.8. If you have to build it yourself, you can use the installation script included in contrib/ like so ./contrib/install_db4.sh . from the root of the repository. Note: You only need Berkeley DB if the wallet is enabled (see the section Disable-Wallet mode elow). Build Bitcoin Core Clone the Bitcoin Core source code and cd into bitcoin git clone https://github.com/bitcoin/bitcoin cd bitcoin

./contrib/install_db4.sh . should be mentioned after cd bitcoin.

merland commented at 7:28 AM on October 27, 2018: contributor

@hebasto Thanks for reviewing. I think I agree to both of your last comments, but I want to push this PR through before it grows even more. I will make a note of your suggestions and then any of us can create a new PR further on.

merland commented at 8:17 AM on October 27, 2018: contributor

Can someone tell me why the AppVeyor build is failing? Seems unrelated...

merland commented at 11:15 AM on October 27, 2018: contributor

@hebasto I reconsidered and have pushed almost all of your suggestions. Thanks.

merland force-pushed on Oct 27, 2018

merland commented at 11:54 AM on October 27, 2018: contributor

Squashed to 1 commit

fanquake commented at 1:15 AM on October 28, 2018: member

utACK c68464f

The changes look fine, however, please update the commit description so that it doesn't include all the messages from the interim commits, and remove any @ mentions (they just lead to spam mentions/emails when downstream projects pull our commits).

Was planning to squash right before merge.

Please don't open PRs with the intent to "cleanup" and squash after review/before merge, as that just means another round of review (less of an issue here, but will always be the case for code changes).

PR's should either be clearly marked [wip] if there is technical and/or code feedback required (that couldn't be discussed in an issue), otherwise, if you open a PR it should be in a ready to merge state.

Various textual improvements in build docs 36c8e68585

merland force-pushed on Oct 28, 2018

merland commented at 5:09 AM on October 28, 2018: contributor

@fanquake Thanks a lot for clearing this up for me. I'm new to both squashing and the project workflow, so this was a good learning experience.

The commit message has been cleaned up now.

fanquake commented at 5:50 AM on October 28, 2018: member

re-utACK 36c8e68

hebasto commented at 6:53 AM on October 28, 2018: member

re-utACK 36c8e68585091d10d8a1d4f97d2db87cd8cedcb2

MarcoFalke merged this on Oct 28, 2018

MarcoFalke closed this on Oct 28, 2018

MarcoFalke referenced this in commit 643b25d093 on Oct 28, 2018

laanwj referenced this in commit d38a5092f1 on Nov 1, 2018

merland deleted the branch on Nov 22, 2018

PastaPastaPasta referenced this in commit 478f840f4a on Jun 27, 2021

PastaPastaPasta referenced this in commit 430dc17ab0 on Jun 28, 2021

PastaPastaPasta referenced this in commit 84f99fcd17 on Jun 29, 2021

PastaPastaPasta referenced this in commit c96e59bed9 on Jul 1, 2021

PastaPastaPasta referenced this in commit d2ed900d7d on Jul 1, 2021

PastaPastaPasta referenced this in commit 411abc541d on Jul 1, 2021

PastaPastaPasta referenced this in commit 0ed7a8622a on Jul 3, 2021

MarcoFalke locked this on Sep 8, 2021