Skip to content
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.

Sign up for GitHub

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

Jump to bottom

doc: Improve TxGraph & co docs #1188

Merged
merged 1 commit into from
Dec 13, 2023
Merged

doc: Improve TxGraph & co docs #1188

merged 1 commit into from
Dec 13, 2023

Conversation

danielabrozzoni
Copy link
Member

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

realeinherjar
realeinherjar reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@realeinherjar realeinherjar left a comment

Choose a reason for hiding this comment

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

Some suggestions

crates/chain/src/lib.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_data_traits.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_data_traits.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
@danielabrozzoni danielabrozzoni self-assigned this Oct 31, 2023
@danielabrozzoni danielabrozzoni force-pushed the doc/tx_graph branch 2 times, most recently from c621b97 to b4ba122 Compare November 2, 2023 16:00
@danielabrozzoni
Copy link
Member Author

Ready for another rounds of reviews, thanks everyone!

@danielabrozzoni danielabrozzoni force-pushed the doc/tx_graph branch 2 times, most recently from 1417b49 to 77b889f Compare November 3, 2023 16:09
@nondiremanuel nondiremanuel mentioned this pull request Nov 5, 2023
Closed
38 tasks
vladimirfomene
vladimirfomene reviewed Nov 6, 2023
View reviewed changes
Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

Just nits.

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
@danielabrozzoni danielabrozzoni force-pushed the doc/tx_graph branch 3 times, most recently from 5e36f67 to b572135 Compare November 6, 2023 13:26
@nondiremanuel nondiremanuel added this to the 1.0.0-alpha.3 milestone Nov 7, 2023
evanlinjin
evanlinjin reviewed Nov 8, 2023
View reviewed changes
crates/chain/src/tx_data_traits.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Show resolved Hide resolved
@danielabrozzoni danielabrozzoni mentioned this pull request Nov 9, 2023
Merged
3 tasks
@danielabrozzoni
Copy link
Member Author

I pushed a few additions as specified in #1109

@nondiremanuel nondiremanuel mentioned this pull request Nov 19, 2023
Closed
54 tasks
@danielabrozzoni danielabrozzoni force-pushed the doc/tx_graph branch 3 times, most recently from 5dc916e to a5e087a Compare November 22, 2023 17:57
@nondiremanuel nondiremanuel mentioned this pull request Dec 2, 2023
Closed
55 tasks
notmandatory
notmandatory approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK a5e087a

It looks like all comments have been addresses and this is a nice improvement to the docs. @LLFourn and @evanlinjin if you don't have any other suggestions can we merge this now?

@LLFourn
Copy link
Contributor

LLFourn commented Dec 6, 2023

There are still points where we refer to types as "structs" or "structures". For example:

/// This struct is created by the [walk_ancestors] method of [TxGraph].

Can just be:

/// Returned by the [walk_ancestors] method of [TxGraph]

This is my only nit. For some reason this redundancy of calling something which will obviously be displayed as a struct a struct makes me seethe.

@danielabrozzoni
Copy link
Member Author

I pushed some final changes to the docs, removing the "struct"/"structure" terms where I found them, but the MSRV CI is broken :(

@notmandatory notmandatory mentioned this pull request Dec 8, 2023
Closed
3 tasks
@notmandatory
Copy link
Member

notmandatory commented Dec 8, 2023 •
edited
Loading

I pushed some final changes to the docs, removing the "struct"/"structure" terms where I found them, but the MSRV CI is broken :(

I just fixed the CI issue in #1236, you can cherry pick it in to this PR or rebase after it gets merged. It's better to merge #1183 instead.

@notmandatory
Copy link
Member

Please rebase now that #1183 has been merged.

@danielabrozzoni
Copy link
Member Author

Done, thanks!

notmandatory
notmandatory approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Re-ACK 0adff9c

@notmandatory notmandatory merged commit b13505c into bitcoindevkit:master Dec 13, 2023
12 checks passed
@nondiremanuel nondiremanuel mentioned this pull request Jan 1, 2024
Closed
24 tasks
@danielabrozzoni danielabrozzoni mentioned this pull request Feb 14, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LLFourn LLFourn LLFourn left review comments

@vladimirfomene vladimirfomene vladimirfomene left review comments

@evanlinjin evanlinjin evanlinjin left review comments

@realeinherjar realeinherjar realeinherjar left review comments

@notmandatory notmandatory notmandatory approved these changes

Assignees

@danielabrozzoni danielabrozzoni

Labels
None yet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha.3
Development

Successfully merging this pull request may close these issues.
7 participants
@danielabrozzoni @LLFourn @notmandatory @vladimirfomene @evanlinjin @realeinherjar @nondiremanuel