Add policy section to docs by tcharding · Pull Request #1127 · rust-bitcoin/rust-bitcoin

tcharding · 2022-07-26T01:25:19Z

In an effort to consolidate knowledge spread out over time in various places on GitHub add a Policy section to CONTRIBUTING.md.

Add initial sections on import statements, errors, rustdocs,attributes, and licensing.

tcharding · 2022-07-26T01:48:26Z

Possible other candidate for inclusion, info on SPDX e.g.,

Files should start with license information as follows:

// SPDX-License-Identifier: CC0-1.0

apoelstra · 2022-07-26T13:54:24Z

Neat! A good start. I'm sure others will chime in with things they think we ought to add.

We should require three ACKs for this PR.

apoelstra

ACK a954f0432a1dc63e71d7eb5018da39fb62c5ce9b

Kixunil

Concept ACK, few suggestions.

Kixunil · 2022-07-26T14:03:14Z

CONTRIBUTING.md

Error types should be non_exhaustive and have private fields unless you're very confident they won't change.
Error types should not commit to implementing traits they may not be able to implement int the future, especially Copy but possibly also Eq and Ord. They should however commit to Send and Sync.
If the error contains source implement Display using write_err!() macro.
If a function consumes costly-to-compute input (allocations are also considered costly) it should return it back in the error type.
Errors should have Error suffix and conditionally implement std::error::Error if they are public.

Should we have a small set of derives that are used on all error types? Clone, Debug, PartialEq?

They should however commit to Send and Sync

I don't know what action is required to achieve this, I imagine other readers of the doc may not either. Can you explain further please?

Debug is the only one I'd guarantee. Clone is very likely for most error types but there could be exception when we want to return a resource instead of consuming it. (OTOH &mut is probably better)

I don't know what action is required to achieve this

Not putting things like Cell or Rc into Error types. Sadly it could happen by accident without the compiler complaining and we may break the user code. Maybe we should have something like this:

// #[allow(unused)] may be required trait AssertSendSync: Send + Sync { } // Perhaps we could also check other traits macro_rules! check_pub_error { ($type:ty) => { impl AssertSendSync for $ty {} } }

Done! ~~#1148~~ Now done in #1743

CONTRIBUTING.md

tcharding · 2022-07-28T03:59:48Z

We should require three ACKs for this PR.

If/when bors comes about we should consider if there is some way to flag PRs that require more acks.

EDIT: I added a label manual merge, bors can check for it before merging. No pressure but is bors ready yet :)

Kixunil · 2022-07-28T11:48:05Z

I'd like some rules that require more acks based on files changed or so, if we have any critical code or policies. Might be reasonably easy to add.

tcharding · 2023-02-10T00:39:33Z

Putting on ice until after release of 0.30.0

tcharding · 2023-03-23T04:33:43Z

I'd like some rules that require more acks based on files changed or so, if we have any critical code or policies. Might be reasonably easy to add.

Just flagging that I took this PR off draft but have not actioned this comment of yours @Kixunil, not sure what to do about it?

apoelstra · 2023-03-23T16:14:43Z

Our current policy is roughly "if one of the 2 ACKers says that they want a 3rd ACK, then we do it". I wouldn't mind codifying this. But I can't think of a simple mechanical rule that would say "you need 3 ACKs if you touch this file" or something like that.

This PR is an example of something that would need 3 ACKs -- though I'm willing to accept @tcharding's PR opening as an implicit ACK, since otherwise it would be hard to get a third. (Unless @sanket1729 wants to chime in on this.)

CONTRIBUTING.md

tcharding · 2023-03-23T23:44:22Z

changes in force push:

added "Attributes" section
fixed typos
Added to #[non_exhaustive] in error: // Add liberally; if the error type may ever have new variants added.

sanket1729

ACK 4d630e0ac386eb26a975429af590ea52219362b0

tcharding · 2023-10-02T03:53:22Z

Removed the mention of check_pub_error, which is from #1743

apoelstra

ACK 539b8ad2b9af34ed33113d6738d6e3779dfa7b5a

Kixunil · 2023-10-03T05:47:06Z

CONTRIBUTING.md

This looks weird :)

Dunno what I was doing when I wrote this, the ;s are wrong too. Fixed and force pushed, thanks.

CONTRIBUTING.md

apoelstra

ACK fe1e8aef8cd24b9695d0effe7fc442a2b8b4ddd0

sanket1729

ACK fe1e8aef8cd24b9695d0effe7fc442a2b8b4ddd0

apoelstra · 2023-10-04T17:22:47Z

Since @Kixunil is around I'll let him ACK as well before merging.

tcharding · 2023-10-10T21:41:11Z

On ice until next release.

yancyribbens · 2023-10-13T08:06:43Z

Is it worth adding mention of how silent overflows should be handled (or not handled)? #2086

apoelstra · 2023-10-13T17:22:39Z

I like the idea of having a project-wide overflow policy. But I have no idea what such a policy should be :). Feerates have different rules from amounts, which have different rules from sizes, etc.

In an effort to consolidate knowledge spread out over time in various places on GitHub add a `Policy` section to `CONTRIBUTING.md`. Add initial sections on import statements, errors, rustdocs,attributes, and licensing.

apoelstra

ACK 3bebecc

tcharding · 2023-11-14T02:15:57Z

Can this go in?

apoelstra · 2023-11-14T22:50:46Z

cc @sanket1729 @Kixunil we need a second ack here

Kixunil

ACK 3bebecc

apoelstra previously approved these changes Jul 26, 2022

View reviewed changes

Kixunil reviewed Jul 26, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 4535f63 July 28, 2022 03:59

tcharding force-pushed the 07-26-policy-docs branch from a954f04 to 4535f63 Compare July 28, 2022 03:59

tcharding added the manual merge label Jul 28, 2022

tcharding mentioned this pull request Jul 28, 2022

Future proof error types #1148

Closed

tcharding force-pushed the 07-26-policy-docs branch from 4535f63 to 2ebecb4 Compare July 28, 2022 23:44

tcharding added the documentation label Aug 5, 2022

tcharding added this to the 0.30.0 milestone Aug 5, 2022

tcharding marked this pull request as draft February 10, 2023 00:39

Kixunil modified the milestones: 0.30.0, 0.31.0 Feb 18, 2023

tcharding force-pushed the 07-26-policy-docs branch from 2ebecb4 to 8578505 Compare March 23, 2023 04:32

tcharding marked this pull request as ready for review March 23, 2023 04:32

apoelstra reviewed Mar 23, 2023

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

apoelstra reviewed Mar 23, 2023

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

apoelstra reviewed Mar 23, 2023

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

tcharding mentioned this pull request Mar 23, 2023

Add check_pub_error macro #1743

Closed

tcharding force-pushed the 07-26-policy-docs branch from 8578505 to 4d630e0 Compare March 23, 2023 23:43

sanket1729 previously approved these changes Mar 23, 2023

View reviewed changes

tcharding dismissed sanket1729’s stale review via 271338d March 23, 2023 23:49

tcharding force-pushed the 07-26-policy-docs branch from 4d630e0 to 271338d Compare March 23, 2023 23:49

tcharding force-pushed the 07-26-policy-docs branch from 903b207 to 307ccf1 Compare October 2, 2023 02:21

tcharding marked this pull request as ready for review October 2, 2023 02:21

tcharding force-pushed the 07-26-policy-docs branch 3 times, most recently from 119a0ea to 539b8ad Compare October 2, 2023 03:53

apoelstra previously approved these changes Oct 2, 2023

View reviewed changes

Kixunil reviewed Oct 3, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via fe1e8ae October 3, 2023 05:59

tcharding force-pushed the 07-26-policy-docs branch from 539b8ad to fe1e8ae Compare October 3, 2023 05:59

apoelstra previously approved these changes Oct 3, 2023

View reviewed changes

sanket1729 previously approved these changes Oct 4, 2023

View reviewed changes

tcharding marked this pull request as draft October 10, 2023 21:41

Add policy section to docs

3bebecc

In an effort to consolidate knowledge spread out over time in various places on GitHub add a `Policy` section to `CONTRIBUTING.md`. Add initial sections on import statements, errors, rustdocs,attributes, and licensing.

tcharding dismissed stale reviews from sanket1729 and apoelstra via 3bebecc October 30, 2023 21:03

tcharding force-pushed the 07-26-policy-docs branch from fe1e8ae to 3bebecc Compare October 30, 2023 21:03

tcharding marked this pull request as ready for review October 30, 2023 21:03

tcharding mentioned this pull request Oct 31, 2023

Improve public re-exports #2120

Merged

apoelstra approved these changes Oct 31, 2023

View reviewed changes

Kixunil approved these changes Nov 15, 2023

View reviewed changes

apoelstra merged commit 50fc63b into rust-bitcoin:master Nov 16, 2023

tcharding deleted the 07-26-policy-docs branch May 22, 2024 23:21

tcharding mentioned this pull request Aug 13, 2025

Move policy stuff to docs directory #4852

Merged

Conversation

tcharding commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Jul 26, 2022

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!

Kixunil Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Kixunil Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 28, 2022 • 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!

Uh oh!

tcharding commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Jul 28, 2022

Uh oh!

tcharding commented Feb 10, 2023

Uh oh!

tcharding commented Mar 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tcharding commented Mar 23, 2023

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 2, 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 Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 4, 2023

Uh oh!

tcharding commented Oct 10, 2023

Uh oh!

yancyribbens commented Oct 13, 2023

Uh oh!

apoelstra commented Oct 13, 2023

Uh oh!

apoelstra left a comment

tcharding commented Jul 26, 2022 •

edited

Loading

tcharding commented Jul 26, 2022 •

edited

Loading

tcharding Jul 28, 2022 •

edited

Loading

tcharding commented Jul 28, 2022 •

edited

Loading

apoelstra commented Mar 23, 2023 •

edited

Loading

tcharding commented Oct 2, 2023 •

edited

Loading