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

Cleanups and fixups #514

Merged
merged 19 commits into from
Aug 1, 2019
Merged

Cleanups and fixups #514

merged 19 commits into from
Aug 1, 2019

Conversation

azilber
Copy link
Contributor

@azilber azilber commented Jul 14, 2019

Changed Bitcoin -> Zcoin verbiage in help texts, and error/info messages.
Omni message should be Exodus.

@ultimaweapon
Copy link
Contributor

Thanks for contribution! Could you please fix the test failed? Here is the logs:

test/crypto_tests.cpp(33): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(46): error in "sha256_testvectors": check hash == out failed
test/crypto_tests.cpp(50): error in "sha256_testvectors": check hash == out failed

@azilber
Copy link
Contributor Author

azilber commented Jul 15, 2019

I'm unable to get tests to run on OSX. Keep getting this error:

$ ./src/test/test_bitcoin --log_level=all --run_test=crypto_tests --color_output=0
unknown location:0: fatal error: in "Test setup": boost::runtime::access_to_missing_argument: There is no argument provided for parameter color_output

*** No errors detected
Test setup error: 

Even when following this: https://stackoverflow.com/questions/39171467/there-is-no-argument-provided-for-parameter-color-output-with-boost-test-and-cte

@azilber
Copy link
Contributor Author

azilber commented Jul 15, 2019

Opened issue 517 regarding not being able to run the test suite. Attempted that on vanilla code.

@reubenyap reubenyap added this to the v0.13.8.3 milestone Jul 17, 2019
Copy link
Contributor

@ultimaweapon ultimaweapon left a comment

Choose a reason for hiding this comment

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

For text I think it should be Zcoin instead of zcoin. @reubenyap what do you think?

@catchingknives
Copy link
Contributor

I agree

@azilber
Copy link
Contributor Author

azilber commented Jul 19, 2019

I also agree, it should always be Zcoin as that seems to be what's most actively used. I've used "zcoin" where it used to be "bitcoin" before, and "Zcoin" where "Bitcoin" was specified, matching case.

So to confirm, you want each, and every occurrence of "zcoin" to be "Zcoin"?

@ultimaweapon
Copy link
Contributor

@azilber Yes for display string change to Zcoin. For other string it depend of what is it (e.g. if it is path name for data directory it should be lower cases).

@azilber
Copy link
Contributor Author

azilber commented Jul 22, 2019

@ultimaweapon I've changed all the user-facing text I could find, and most of the comments as well. I left out the obvious and the ambiguous, as well as (obviously) the command related comments. None of the functions/methods were touched.

@reubenyap
Copy link
Member

@azilber So I understand that Bitcoin refers to the network while bitcoin refers to the units, The meaning changes a little depending on the capitalization, did you check whether it's following this ?

@azilber
Copy link
Contributor Author

azilber commented Jul 22, 2019

@reubenyap No it does not. I have Zcoin (for the network) and Zcoins (for the units). This only relates to user facing info.

For example:
original:
\"fee\" : \"n.nnnnnnnn\", (string) the transaction fee in zcoins\n"

new:
\"fee\" : \"n.nnnnnnnn\", (string) the transaction fee in Zcoins\n"

Then there's stuff like:
"your Zcoins from being stolen by malware infecting your computer."

Those are actually not too many, but at times it can be somewhat ambiguous. I think it's really a matter of preference. Which one do you prefer?

@reubenyap
Copy link
Member

@reubenyap No it does not. I have Zcoin (for the network) and Zcoins (for the units). This only relates to user facing info.

For example:
original:
\"fee\" : \"n.nnnnnnnn\", (string) the transaction fee in zcoins\n"

new:
\"fee\" : \"n.nnnnnnnn\", (string) the transaction fee in Zcoins\n"

Then there's stuff like:
"your Zcoins from being stolen by malware infecting your computer."

Those are actually not too many, but at times it can be somewhat ambiguous. I think it's really a matter of preference. Which one do you prefer?

Sorry about the confusion but I think I prefer to keep the same wording to refer to zcoin when you're dealing with the currency and Zcoin when it refers to the project/protocol/network!

Thanks @azilber !

@azilber
Copy link
Contributor Author

azilber commented Jul 24, 2019

This last commit should do the trick. Please review.

doc/debug-vs2017.txt Outdated Show resolved Hide resolved
src/exodus/rpc.cpp Outdated Show resolved Hide resolved
src/exodus/rpctx.cpp Outdated Show resolved Hide resolved
src/exodus/rpctx.cpp Outdated Show resolved Hide resolved
src/exodus/rpctx.cpp Outdated Show resolved Hide resolved
src/qt/paymentserver.cpp Outdated Show resolved Hide resolved
src/test/sigma_partialspend_mempool_tests.cpp Outdated Show resolved Hide resolved
src/test/test_bitcoin.cpp Show resolved Hide resolved
src/wallet/wallet.h Outdated Show resolved Hide resolved
src/wallet/wallet.h Outdated Show resolved Hide resolved
@azilber
Copy link
Contributor Author

azilber commented Jul 25, 2019

@ultimaweapon There's a few unresolved items, please check.

@ultimaweapon
Copy link
Contributor

@azilber thanks for your fixes! For the message Cannot start zcoin: click-to-pay handler what it mean is it was tried to start handler for zcoin:// URI and it failed.

@azilber
Copy link
Contributor Author

azilber commented Jul 26, 2019

Ok, this should be the last of them. There might be still some random stragglers, but I think I caught all of the obvious ones.

ultimaweapon
ultimaweapon previously approved these changes Jul 31, 2019
@catchingknives catchingknives modified the milestones: v0.13.8.3, v0.13.8.4 Jul 31, 2019
src/qt/locale/bitcoin_da.ts Outdated Show resolved Hide resolved
src/qt/locale/bitcoin_it.ts Outdated Show resolved Hide resolved
src/qt/locale/bitcoin_ja.ts Outdated Show resolved Hide resolved
src/qt/locale/bitcoin_tr.ts Outdated Show resolved Hide resolved
src/qt/locale/bitcoin_zh_TW.ts Outdated Show resolved Hide resolved
src/qt/tradehistorydialog.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@thebevrishot thebevrishot left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution.

Copy link
Contributor

@a-bezrukov a-bezrukov left a comment

Choose a reason for hiding this comment

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

Titanic work! Thanks!

@a-bezrukov a-bezrukov merged commit 0350571 into firoorg:master Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants