Improve README by tcharding · Pull Request #1736 · rust-bitcoin/rust-bitcoin

tcharding · 2023-03-22T22:43:57Z

Improve the readme by doing:

Patch 1: Enforce column width to 100
Patch 2: Update policy on altcoin support (or lack of)

Excuse the OCD leading to patch one, its formatting only no content change.

apoelstra

ACK 9e0332b51361064720856328aec66e3ac4ab53e4

Kixunil

I'm confused why there's a second PR that says 1.41. If there are two I'd prefer one o go after another to minimize churn around versions.

Kixunil · 2023-03-28T19:33:11Z

README.md

I don't mind but I'd like to see line breaks after phrases or logical parts of complicated phrases (Something long and something else also long.) It's pretty convenient to edit in vim :)

Sure thing, will add.

Made a couple of small changes to patch 1, by all means let me know if you think it needs more - happy to learn your preferred style for markdown files.

It's not something I'm obsessed about, just a minor suggestion. What I meant was writing like this:

This library **must not** be used for consensus code (i.e. fully validating blockchain data). It technically supports doing this, but doing so is very ill-advised because there are many deviations, known and unknown, between this library and the Bitcoin Core reference implementation. In a consensus based cryptocurrency such as Bitcoin it is critical that all parties are using the same rules to validate data, and this library is simply unable to implement the same rules as Core.

But I'd rather hear opinions of others before doing change like this.

Kixunil · 2023-03-28T19:35:48Z

README.md

Given we don't have such enums (anymore?) we should just drop this and say we don't support shitcoins. ChainHash is closest to supporting shitcoins, probably not worth the trouble.

Yeah, I think so. The altcoin landscape has changed dramatically since 2014 anyway. This was written back when most shitcoins were clonecoins and "supporting" them was a matter of tweaking a couple constants. Nowadays the most popular things are all pretty unique. Even litecoin has Confidential Transactions.

Removed as an additional patch.

Kixunil · 2023-03-28T19:37:14Z

README.md

Why this still mentions 1.41/1.47?

Needed rebase, done now.

Kixunil · 2023-03-28T19:38:18Z

README.md

Either s/will/may/ or explicitly mention it only applies to serde feature.

Removed syn since its no longer needed.

tcharding · 2023-03-29T03:26:08Z

I'm confused why there's a second PR that says 1.41. If there are two I'd prefer one o go after another to minimize churn around versions.

Not sure what you are referring to. Hopefully the rebase of this is all that was needed.

Kixunil · 2023-03-29T08:16:56Z

README.md

It's not something I'm obsessed about, just a minor suggestion. What I meant was writing like this:

This library **must not** be used for consensus code (i.e. fully validating blockchain data). It technically supports doing this, but doing so is very ill-advised because there are many deviations, known and unknown, between this library and the Bitcoin Core reference implementation. In a consensus based cryptocurrency such as Bitcoin it is critical that all parties are using the same rules to validate data, and this library is simply unable to implement the same rules as Core.

But I'd rather hear opinions of others before doing change like this.

Kixunil · 2023-03-29T08:18:45Z

README.md

I meant to say something like:

To build with the MSRV you will need to pin serde if you have the feature enabled:

Integrated this suggestion, with minor changes - please re-check.

I don't think you wanted to do it in a commit modifying doc about altcoins, did you?

Kixunil · 2023-03-29T08:24:59Z

README.md

I actually liked the explanation so I'd keep it in. Perhaps:

Since altcoin landscape includes projects which frequently appear and disappear, and are poorly designed anyway we do not support any altcoins. Supporting Bitcoin properly is already difficult enough and we don't want to increase maintenance burden and decrease API stability by adding other coins.

Our code is public domain so by all means fork it and go wild :)

Used (with minor changes grammar), thanks.

tcharding · 2023-03-29T22:06:52Z

It's not something I'm obsessed about, just a minor suggestion.

Bizzare, there is no box to reply below the actual comment. The suggested form is bad for me because I cannot tell by looking at it what the rules are - I'm a sucker for explicit rules so I can quickly parse something and tell if it conforms.

Perhaps you could explain what exactly makes you life harder in the editor then i'll be able to formulate the rules. (We use different editors so workflow may be different.)

tcharding · 2023-03-29T22:09:33Z

I think we can merge this and continue a discussion on the best form of markdown files that suits everyone, ok with you @Kixunil?

Kixunil

I wouldn't mind but if you put so much effort into proper commits I guess you you will want to fix the last nit. :)

Kixunil · 2023-03-30T06:51:04Z

README.md

I don't think you wanted to do it in a commit modifying doc about altcoins, did you?

sanket1729

utACK 81c61b988f00dfc9c3792847f718fc02001db04e

tcharding · 2023-03-30T21:50:18Z

I wouldn't mind but if you put so much effort into proper commits I guess you you will want to fix the last nit. :)

Thanks for noticing the mistake and the fact that I put a lot of effort into well formed commits. This was definitely a rebase error - not closely re-reading all my diffs is one of my dev failures. Will fix (the current PR, and hopefully one day the dev-failure :)

tcharding · 2023-03-30T21:56:08Z

Changes in force push:

Put the pin changes in the correct patch
Added a "the" to improve grammar in the last patch

junderw · 2023-03-30T22:24:38Z

README.md

tcharding · 2023-03-31T00:36:42Z

If it is non-controversial please let #1696 go in before this one.

apoelstra

ACK 35a72470d018636d9686b653a491b30df0300c88

6c61e10 Fix pinning (schemars and MSRV) (Tobin C. Harding) c8e38d6 hashes: Implement JsonSchema for sha256t::Hash<T> (Tobin C. Harding) Pull request description: This has grown due to now including pinning work also done in #1736, I decided to do this because the PRs conflict and doing it all here saves accidentally getting out of sync. And #1764 requires this PR. - Patch 1 is unchanged - Patch 2 now fixes pinning in bitcoin and hashes CI scripts and in the docs of both as well as the manifest stuff relating to `schemars` - phew. Fix: #1687 ACKs for top commit: Kixunil: ACK 6c61e10 apoelstra: ACK 6c61e10 Tree-SHA512: eae4aa9700817bab6ad444e07709e8b1a4ffb1625e08be6ba399abde38bf6f8e5ea216a0836e2e26dfaddc76c392802cd016738ea6e753a1bca2584d9d2a9796

The readme has gotten a bit messy with various contributors using different collum width. Make it all 100 so that new contributors have some chance of keeping it tidy. On top of that, use "natural" line breaks if it assists reading/editing i.e., don't be dogmatic about column length.

We don't altcoin, state it as such.

Kixunil

Enthusiastic ACK 2f7bf1e

I wanted this for a while. :)

apoelstra

ACK 2f7bf1e

apoelstra previously approved these changes Mar 23, 2023

View reviewed changes

Kixunil requested changes Mar 28, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via 5458724 March 29, 2023 03:19

tcharding force-pushed the 03-23-pin branch from 9e0332b to 5458724 Compare March 29, 2023 03:19

tcharding changed the title ~~Improve README (pinning section)~~ Improve README Mar 29, 2023

tcharding force-pushed the 03-23-pin branch 2 times, most recently from 9a12ce2 to 7cbdb87 Compare March 29, 2023 03:24

tcharding requested a review from Kixunil March 29, 2023 03:26

Kixunil reviewed Mar 29, 2023

View reviewed changes

tcharding force-pushed the 03-23-pin branch from 7cbdb87 to 81c61b9 Compare March 29, 2023 22:09

Kixunil reviewed Mar 30, 2023

View reviewed changes

README.md Outdated

Copy link
Copy Markdown

Collaborator

Kixunil Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you wanted to do it in a commit modifying doc about altcoins, did you?

sanket1729 previously approved these changes Mar 30, 2023

View reviewed changes

tcharding dismissed sanket1729’s stale review via 40417b7 March 30, 2023 21:53

tcharding force-pushed the 03-23-pin branch 2 times, most recently from 40417b7 to 35a7247 Compare March 30, 2023 21:55

junderw reviewed Mar 30, 2023

View reviewed changes

README.md Outdated

Copy link
Copy Markdown

Contributor

junderw Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

tcharding mentioned this pull request Mar 31, 2023

Remove schemars pin from manifest #1696

Merged

apoelstra previously approved these changes Mar 31, 2023

View reviewed changes

tcharding added 2 commits May 4, 2023 16:09

readme: Document that we do not support altcoins

2f7bf1e

We don't altcoin, state it as such.

tcharding dismissed apoelstra’s stale review via 2f7bf1e May 4, 2023 06:10

tcharding force-pushed the 03-23-pin branch from 35a7247 to 2f7bf1e Compare May 4, 2023 06:10

tcharding added the documentation label May 4, 2023

tcharding added the trivial Obvious, easy and quick to review (few lines or doc-only...) label May 4, 2023

Kixunil approved these changes May 4, 2023

View reviewed changes

apoelstra approved these changes May 4, 2023

View reviewed changes

apoelstra merged commit d5f4791 into rust-bitcoin:master May 4, 2023

tcharding deleted the 03-23-pin branch May 4, 2023 22:06

Conversation

tcharding commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 29, 2023

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 31, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

tcharding commented Mar 22, 2023 •

edited

Loading

tcharding Mar 29, 2023 •

edited

Loading

tcharding commented Mar 29, 2023 •

edited

Loading

tcharding commented Mar 30, 2023 •

edited

Loading