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.

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

Use PACKAGE_NAME in messages rather than hardcoding "Bitcoin Core" #26142

Merged
merged 1 commit into from Oct 19, 2022
Merged

Use PACKAGE_NAME in messages rather than hardcoding "Bitcoin Core" #26142

merged 1 commit into from Oct 19, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 20, 2022

Usually, we do not hardcode "Bitcoin Core" in the user-faced messages.

See:

Also grammar has been improved -- singular instead of plural.

jarolrod
jarolrod reviewed Sep 21, 2022
View reviewed changes
Copy link
Member

@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.

This doesn't work as is
Screen Shot 2022-09-20 at 11 20 00 PM

@hebasto hebasto marked this pull request as draft September 21, 2022 10:09
@hebasto hebasto marked this pull request as ready for review September 21, 2022 11:00
@hebasto
Copy link
Member Author

hebasto commented Sep 21, 2022

@jarolrod

This doesn't work as is

Thanks! Updated.

luke-jr
luke-jr reviewed Sep 22, 2022
View reviewed changes
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated
return strprintf(_("%s request to listen on port %u. This port is considered \"bad\" and "
"thus it is unlikely that any Bitcoin Core peers connect to it. See "
return strprintf(_("%s requests to listen on port %u. This port is considered \"bad\" and "
"thus it is unlikely that any %s peers connect to it. See "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"thus it is unlikely that any %s peers connect to it. See "
"thus it is unlikely that any peers will connect to it. See "

No reason to name the software specifically here?

Copy link
Member

Choose a reason for hiding this comment

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

true

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, we are touching this, but it isn’t a part of the pr’s goal to address something like this; seems appropriate for a follow-up if someone wants to suggest this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason to name the software specifically here?

Updated.

src/qt/forms/optionsdialog.ui
@@ -286,7 +286,7 @@
<item>
<widget class="QLineEdit" name="externalSignerPath">
<property name="toolTip">
<string>Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string>
<string>Full path to a %1 compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string>
Copy link
Member

Choose a reason for hiding this comment

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

ACK

src/qt/optionsdialog.cpp
@@ -91,7 +91,9 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
ui->thirdPartyTxUrls->setVisible(false);
}

#ifndef ENABLE_EXTERNAL_SIGNER
#ifdef ENABLE_EXTERNAL_SIGNER
ui->externalSignerPath->setToolTip(ui->externalSignerPath->toolTip().arg(PACKAGE_NAME));
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice if there was a BIP we could specify here...

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 23, 2022 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@hebasto
Copy link
Member Author

hebasto commented Sep 27, 2022

Updated d895eb2 -> b147322 (pr26142.02 -> pr26142.03, diff):

luke-jr
luke-jr approved these changes Sep 28, 2022
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK

@hebasto
Copy link
Member Author

hebasto commented Oct 18, 2022

Friendly ping @jarolrod.

jarolrod
jarolrod approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@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.

ACK b147322

No visual difference between pr and master, as expected.

pr master
Screen Shot 2022-10-19 at 2 39 44 AM Screen Shot 2022-10-19 at 2 55 41 AM

@maflcko maflcko merged commit bbe2655 into bitcoin:master Oct 19, 2022
@hebasto hebasto deleted the 220920-package branch October 19, 2022 17:19
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2022
Merge bitcoin#26142: Use PACKAGE_NAME in messages rather than hardcod…
803371c 
…ing "…
@bitcoin bitcoin locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@luke-jr luke-jr luke-jr approved these changes

@jarolrod jarolrod jarolrod approved these changes

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@hebasto @DrahtBot @luke-jr @jarolrod @maflcko @glozow