Contributing.md#587
Conversation
|
@TheBlueMatt @Kixunil addressed your comments with a separate commit 4e64db1e316ecd02d5d585f5561d90dd5544a981 |
sgeisler
left a comment
There was a problem hiding this comment.
Left some minor nits, looks good overall. Thx for tackling this issue :)
The only major thing we should talk about more are security disclosures imo.
|
ACK |
sgeisler
left a comment
There was a problem hiding this comment.
General ACK, just please revert the changes to the dependency versions (was probably just an oversight).
sgeisler
left a comment
There was a problem hiding this comment.
ACK 92bb0e503f34813ddb90bc4ad26a5a81d79571c4
|
@apoelstra did all of that + tried to make CI scripts to match the guidelines as you described |
|
@sanket1729 did that |
|
@Kixunil pls re-ACK. I updated the file according to Andrew's review. We have removed derive section to be added later as a separate PR after a proper discussion. |
|
@apoelstra shell we wait for more ACKs here - or this can be merged already? |
|
Let's wait for one more ACK. It's not going to conflict with anything. |
CONTRIBUTING.md
Outdated
| Communication about Rust Bitcoin happens primarily in | ||
| [#bitcoin-rust](https://web.libera.chat/?channel=#bitcoin-rust) IRC chat on | ||
| [Libera](https://libera.chat/) with the logs available at | ||
| <http://gnusha.org/rust-bitcoin/>. |
There was a problem hiding this comment.
It looks the logs at http://gnusha.org/rust-bitcoin/ didn't switch to libera because they are not working since about June 2021
There was a problem hiding this comment.
Yes. We have to contact Bryan Bishop to do that.
There was a problem hiding this comment.
Actually I just checked and it looks liek https://gnusha.org/bitcoin-rust/ is up to date.
So actually, we should change our CONTRIBUTING file and our IRC /topic.
There was a problem hiding this comment.
May I do that with a fixup commit not to make re-review the whole PR for others?
There was a problem hiding this comment.
Actually I just checked and it looks liek https://gnusha.org/bitcoin-rust/ is up to date.
Is it correct that days are there, but they are empty (except from log-start/log-end) up to June 2021?
There was a problem hiding this comment.
There is very little activity on the channel, but the logs do not appear empty to me.
There was a problem hiding this comment.
Ah sorry I was watching http://gnusha.org/rust-bitcoin/ while you linked http://gnusha.org/bitcoin-rust/ (rust and bitcoin switched)
|
@apoelstra @Kixunil @RCasatta @sanket1729 I fixed the reference to IRC logs as a separate fixup commit 31c4983. Pls re-ack |
|
Sorry for disturbing, @RCasatta @sanket1729, but this needs just a review of a single fixup commit in 31c4983 since your last reviews of this PR. We don't want to merge without your ACKs since you did a lot of commenting here before. |
|
ACK 31c4983 |
|
Seems like we have 4 ACKs now (@RCasatta did it in a wrong way not clicking GitHub button, but I checked with bitcoin merge tool, it still sees/uses it). @apoelstra wouldn't you mind about merging it? |
|
I noticed one should take care using the word ACK in phrases not meant to be for acking otherwise the tool will take it for the "ACKs for top commit:" section even if it's not intended. |
|
I think, as long as you don't mention the top commit in the same line, you should be okay? |
|
yes, if you don't mention the top commit you are fine, pretty rare false positive, so it happened this time :) |
1b2dbd7 0.28.0-rc.1 release (Dr Maxim Orlovsky) Pull request description: We still have quite a few issues and PRs pending to be addressed/merged before full 0.28 release: see https://github.com/rust-bitcoin/rust-bitcoin/milestone/10 Most important, we have to find a way to address #777; additionally it will be desirable to get #587, #786, #776. We also have quite of work to write CHANGELOG in #785 Nevertheless I propose to start with a `beta-1` subrelease to unlock ability for `miniscript` release and downstream testing. The Taproot API looks final, and the pending PRs are addresses internal issues/bugs which is perfectly fine for beta releases. ACKs for top commit: Kixunil: ACK 1b2dbd7 Tree-SHA512: a7bd6dd3e7a489f7fd758fb0d7a60103bfe0cdf88997b2163f163841fa1c12e7b31fa8f03b9f817a671379578dbc10a2ca583f49b64d2d2de4a9ebb57b2b9bd5
69f44d9301c54168e9eaaf046db3d478e6739173 Manually implement Debug for SerializedSignature (Tobin Harding) 26921a31b8a030fba1141aba9ee61201c21b2d6a Add lints to catch missing traits (Tobin Harding) 35556e22f20a47341a6f5c3c68416713fdf354c3 Remove useless call to format (Tobin Harding) 0ad414a9825f8a3e8d634ee3207e44538497b09b Remove unneeded return statements (Tobin Harding) Pull request description: We can use the linters to help us catch type definitions that are missing 'standard' derives. 'standard' is project defined to be - Copy - Clone - Debug - PartialEq and Eq - PartialOrd and Ord - Hash (I've assumed this to be true based on the code and an open [PR](rust-bitcoin/rust-bitcoin#587) in rust-bitcoin.) While neither Rustc nor Clippy can find all of these, Rustc can warn for missing `Copy` and `Debug` implementations and these warnings can assist us find types that may need additional derives. First two patches are trivial Clippy fixes in preparation for using the linter to improve type definitions crate wide. Patch 3 adds ``` #![warn(missing_copy_implementations)] #![warn(missing_debug_implementations)] ``` and fixes newly emitted warnings. ACKs for top commit: thomaseizinger: ACK 69f44d9301c54168e9eaaf046db3d478e6739173 apoelstra: ACK 69f44d9301c54168e9eaaf046db3d478e6739173 Tree-SHA512: 18f2c52d207f962ef7d6749a57a35e48eb18a18fac82d4df4ff3dce549b69661cb27f66c4cae516ae5477f5b919d9197f70a5c924955605c73f8545f430c3b42
69f44d9301c54168e9eaaf046db3d478e6739173 Manually implement Debug for SerializedSignature (Tobin Harding) 26921a31b8a030fba1141aba9ee61201c21b2d6a Add lints to catch missing traits (Tobin Harding) 35556e22f20a47341a6f5c3c68416713fdf354c3 Remove useless call to format (Tobin Harding) 0ad414a9825f8a3e8d634ee3207e44538497b09b Remove unneeded return statements (Tobin Harding) Pull request description: We can use the linters to help us catch type definitions that are missing 'standard' derives. 'standard' is project defined to be - Copy - Clone - Debug - PartialEq and Eq - PartialOrd and Ord - Hash (I've assumed this to be true based on the code and an open [PR](rust-bitcoin/rust-bitcoin#587) in rust-bitcoin.) While neither Rustc nor Clippy can find all of these, Rustc can warn for missing `Copy` and `Debug` implementations and these warnings can assist us find types that may need additional derives. First two patches are trivial Clippy fixes in preparation for using the linter to improve type definitions crate wide. Patch 3 adds ``` #![warn(missing_copy_implementations)] #![warn(missing_debug_implementations)] ``` and fixes newly emitted warnings. ACKs for top commit: thomaseizinger: ACK 69f44d9301c54168e9eaaf046db3d478e6739173 apoelstra: ACK 69f44d9301c54168e9eaaf046db3d478e6739173 Tree-SHA512: 18f2c52d207f962ef7d6749a57a35e48eb18a18fac82d4df4ff3dce549b69661cb27f66c4cae516ae5477f5b919d9197f70a5c924955605c73f8545f430c3b42
We've being discussing that a
CONTRIBUTING.mdguidelines are needed for some time and I took an effort to propose it. The main text is taken fromrust-lightningversion (with some additions), but it is extended on the topics we were discussing in multiple issues previously:rustfmtFiles are not formatted with latest rustfmt #172Each of these issue-related amendments to the basic (modified) version of rust-lightning contributing guidelines are made with a separate commit.