Skip to content

Refactor TransactionDesc::FormatTxStatus and TransactionStatus #552

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
Merged
merged 5 commits into from
Apr 15, 2022
Merged

Refactor TransactionDesc::FormatTxStatus and TransactionStatus #552

merged 5 commits into from
Apr 15, 2022

Conversation

@w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Feb 18, 2022 •
edited
Loading

This PR implements the changes suggested in #538 (comment) .

. remove redundant scope, rename nDepth -> depth, remove unused parameters and add translator comments in TransactionDesc::FormatTxStatus()
. Use member initializers and remove unused field qint64 TransactionStatus::open_for in TransactionStatus.

Closes #538

jonatack
jonatack reviewed Feb 19, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Code review ACK cbf23e4 modulo a couple comments

@w0xlt w0xlt force-pushed the pr24067-follow-ups branch from cbf23e4 to b0ffe4e Compare February 19, 2022 02:06
jonatack
jonatack reviewed Feb 20, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Code review ACK b0ffe4e

hebasto
hebasto reviewed Feb 20, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 0b234f9.

In 0b234f9 "qt: remove redundant scope":

  • it would be nice to rename s/nDepth/depth/
  • parameters const interfaces::WalletTx& wtx and int numBlocks are not used, and could be dropped (maybe in a separated commit)
  • if you feel comfortable doing that, translator comments are always welcome :)

In b0ffe4e "qt: refactor TransactionStatus":

  • removing of open_for member could be split into a separated commit
  • bool needsUpdate is still uninitialized
  • maybe put into commit message something like this " qt, refactor: Use member initializers in TransactionStatus"?

Also could you please, in addition to the link to #538, add actual descriptions of this PR changes to OP, because it goes into a merge commit message.

@w0xlt w0xlt force-pushed the pr24067-follow-ups branch 2 times, most recently from 25fb10d to 064bbdf Compare February 21, 2022 19:17
@w0xlt
Copy link
Contributor Author

w0xlt commented Feb 21, 2022

@hebasto I have addressed all of your suggestions. Thanks.

promag
promag approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK b279331

jarolrod
jarolrod reviewed Feb 23, 2022
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

concept ACK

w0xlt added 3 commits February 24, 2022 13:47
@w0xlt
qt, refactor: remove redundant scope in `TransactionDesc::FormatTxSta…
b1bc143 
…tus()`
@w0xlt
scripted-diff: rename nDepth -> depth
045f8d0 
-BEGIN VERIFY SCRIPT-
s() { sed -i -e 's/nDepth/depth/g' $(git grep -l 'nDepth' $1); }
s src/qt/transactiondesc.cpp
-END VERIFY SCRIPT-
@w0xlt
qt, refactor: remove unused parameters in `TransactionDesc::FormatTxS…
ad6aded 
…tatus()`
@w0xlt w0xlt force-pushed the pr24067-follow-ups branch 2 times, most recently from 9f5a419 to 2c5dbf2 Compare February 24, 2022 17:31
@hebasto
Copy link
Member

hebasto commented Apr 12, 2022

Almost ACK 2c5dbf2 except for 62ab92b "qt, refactor: add translator comments in TransactionDesc::FormatTxStatus()".

If we are adding translator comments, we should also strive to prevent splitting of strings into less meaningful parts.

Suggesting to separate that commit into a dedicated PR, as all of other commits are good to be merged.

@w0xlt Please name this PR in a bit meaningful way because the PR title goes into the project commit history.

@w0xlt w0xlt changed the title PR24067 follow-ups Format TransactionDesc::FormatTxStatus and TransactionStatus Apr 13, 2022
@w0xlt w0xlt force-pushed the pr24067-follow-ups branch from 2c5dbf2 to 343f83d Compare April 13, 2022 15:28
@w0xlt w0xlt mentioned this pull request Apr 13, 2022
Merged
@w0xlt
Copy link
Contributor Author

w0xlt commented Apr 13, 2022

@hebasto the mentioned commit was moved to PR #583.

@hebasto hebasto changed the title Format TransactionDesc::FormatTxStatus and TransactionStatus Refactor TransactionDesc::FormatTxStatus and TransactionStatus Apr 13, 2022
hebasto
hebasto approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 343f83d, I have reviewed the code and it looks OK, I agree it can be merged.

@hebasto
Copy link
Member

hebasto commented Apr 14, 2022 •
edited
Loading

@jonatack @jarolrod @promag

Mind having another (final?) look?

@MarcoFalke This PR addresses your #538 (comment). Does it look ok?

jarolrod
jarolrod approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Code Review ACK 343f83d

Thanks for working on this refactoring. I have reviewed the code and agree that it can be merged.

@hebasto hebasto merged commit 7190de9 into bitcoin-core:master Apr 15, 2022
@w0xlt w0xlt deleted the pr24067-follow-ups branch April 16, 2022 06:26
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 18, 2022
@hebasto
Merge bitcoin-core/gui#552: Refactor `TransactionDesc::FormatTxStatus…
5c6c60d 
…` and ` TransactionStatus`

343f83d qt, refactor: Use member initializers in TransactionStatus (w0xlt)
66d58ad qt, refactor: remove unused field `qint64 TransactionStatus::open_for` (w0xlt)
ad6aded qt, refactor: remove unused parameters in `TransactionDesc::FormatTxStatus()` (w0xlt)
045f8d0 scripted-diff: rename nDepth -> depth (w0xlt)
b1bc143 qt, refactor: remove redundant scope in `TransactionDesc::FormatTxStatus()` (w0xlt)

Pull request description:

  This PR implements the changes suggested in bitcoin-core/gui#538 (comment) .

  . remove redundant scope, rename `nDepth` -> `depth`, remove unused parameters and add translator comments in `TransactionDesc::FormatTxStatus()`
  .  Use member initializers and remove unused field `qint64 TransactionStatus::open_for` in `TransactionStatus`.

  Closes bitcoin-core/gui#538

ACKs for top commit:
  hebasto:
    ACK 343f83d, I have reviewed the code and it looks OK, I agree it can be merged.
  jarolrod:
    Code Review ACK bitcoin-core/gui@343f83d

Tree-SHA512: cc7333d85b7eb731aa8cdd2d8dfc707341532c93e1b5e3858e8341446cf055ba055b601f9662e8d4602726b1bedf13149c46256a60a0ce1a562f94c9986d945a
hebasto added a commit that referenced this pull request Jun 2, 2022
@hebasto
Merge #583: Add translator comments to TransactionDesc::FormatTxStatus
b11ab25 
8cfb562 qt, refactor: add translator comments in `TransactionDesc::FormatTxStatus()` (w0xlt)

Pull request description:

  This PR adds translator comments to `TransactionDesc::FormatTxStatus` as suggested in #552 (comment) and #552 (comment).

ACKs for top commit:
  hebasto:
    ACK 8cfb562

Tree-SHA512: 2c44b915e6309508f34fc22bb90e3d88ad32ed82fdb3a395f7c6716941edc1b311991140d28e838ad622a7484ed86aedd25e55674857fec8716d9575aed25fa0
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 3, 2022
@hebasto
Merge bitcoin-core/gui#583: Add translator comments to `TransactionDe…
243c842 
…sc::FormatTxStatus`

8cfb562 qt, refactor: add translator comments in `TransactionDesc::FormatTxStatus()` (w0xlt)

Pull request description:

  This PR adds translator comments to `TransactionDesc::FormatTxStatus` as suggested in bitcoin-core/gui#552 (comment) and bitcoin-core/gui#552 (comment).

ACKs for top commit:
  hebasto:
    ACK 8cfb562

Tree-SHA512: 2c44b915e6309508f34fc22bb90e3d88ad32ed82fdb3a395f7c6716941edc1b311991140d28e838ad622a7484ed86aedd25e55674857fec8716d9575aed25fa0
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+4 more reviewers

@jonatack jonatack jonatack left review comments

@promag promag promag approved these changes

@jarolrod jarolrod jarolrod approved these changes

@Gemk83 Gemk83 Gemk83 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PR24067 follow-ups

6 participants

@w0xlt @hebasto @jonatack @promag @jarolrod @Gemk83