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
Improve README #1736
Improve README #1736
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.
ACK 9e0332b
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'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.
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 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
:)
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.
Sure thing, will add.
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.
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.
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.
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.
README.md
Outdated
considered on a case-by-case basis, as the altcoin landscape includes projects which [frequently | ||
appear and disappear, and are poorly designed | ||
anyway](https://download.wpsoftware.net/bitcoin/alts.pdf) and keeping the codebase maintainable is a | ||
large priority. |
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.
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.
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.
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.
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.
Removed as an additional patch.
README.md
Outdated
@@ -73,12 +73,17 @@ For more information please see `./CONTRIBUTING.md`. | |||
This library should always compile with any combination of features (minus `no-std`) on **Rust | |||
1.41.1** or **Rust 1.47** with `no-std`. |
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 this still mentions 1.41/1.47?
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.
Needed rebase, done now.
README.md
Outdated
@@ -73,12 +73,17 @@ For more information please see `./CONTRIBUTING.md`. | |||
This library should always compile with any combination of features (minus `no-std`) on **Rust | |||
1.41.1** or **Rust 1.47** with `no-std`. | |||
|
|||
To build with the MSRV you will need to pin some dependencies (also for `no-std`): | |||
However, if you want to use the library with either of these Rust versions, you will need to pin a |
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.
Either s/will/may/
or explicitly mention it only applies to serde
feature.
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.
Removed syn
since its no longer needed.
9a12ce2
to
7cbdb87
Compare
Not sure what you are referring to. Hopefully the rebase of this is all that was needed. |
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.
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.
README.md
Outdated
@@ -72,11 +72,15 @@ For more information please see `./CONTRIBUTING.md`. | |||
This library should always compile with any combination of features on **Rust 1.48.0**. | |||
|
|||
To build with the MSRV you will need to pin some dependencies: |
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 meant to say something like:
To build with the MSRV you will need to pin
serde
if you have the feature enabled:
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.
Integrated this suggestion, with minor changes - please re-check.
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 don't think you wanted to do it in a commit modifying doc about altcoins, did you?
README.md
Outdated
In general, things that improve cross-chain compatibility (e.g. support for cross-chain atomic | ||
swaps) are more likely to be accepted than things which support only a single blockchain. | ||
|
||
We do not support altcoins, our code is Creative Commons so by all means fork it and go wild :) |
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 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 :)
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.
Used (with minor changes grammar), thanks.
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.) |
I think we can merge this and continue a discussion on the best form of markdown files that suits everyone, ok with you @Kixunil? |
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 wouldn't mind but if you put so much effort into proper commits I guess you you will want to fix the last nit. :)
README.md
Outdated
@@ -72,11 +72,15 @@ For more information please see `./CONTRIBUTING.md`. | |||
This library should always compile with any combination of features on **Rust 1.48.0**. | |||
|
|||
To build with the MSRV you will need to pin some dependencies: |
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 don't think you wanted to do it in a commit modifying doc about altcoins, did you?
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.
utACK 81c61b9
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 :) |
40417b7
to
35a7247
Compare
Changes in force push:
|
Since the altcoin landscape includes projects which [frequently appear and disappear, and are poorly | ||
designed anyway](https://download.wpsoftware.net/bitcoin/alts.pdf) we do not support any altcoins. | ||
Supporting Bitcoin properly is already difficult enough and we do not want to increase the | ||
maintenance burden and decrease API stability by adding support for other coins. |
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.
ACK
If it is non-controversial please let #1696 go in before this 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.
ACK 35a7247
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.
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.
Enthusiastic ACK 2f7bf1e
I wanted this for a while. :)
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.
ACK 2f7bf1e
Improve the readme by doing:
Excuse the OCD leading to patch one, its formatting only no content change.