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

[Docs] Small updates to getrawtransaction description #15332

Merged
merged 1 commit into from Feb 5, 2019

Conversation

Projects
None yet
5 participants
@amitiuttarwar
Copy link
Contributor

commented Feb 3, 2019

As per review comments on #15159

@fanquake fanquake added the Docs label Feb 3, 2019

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

Question about whitespace 😝-
There seems to be an implicit convention of indenting the first line(s) of the RPCHelpMan descriptions and I'm wondering why? The changes look recent but I didn't find anything relevant in the style guide. If this is intentional, maybe I can add something there?

Ex: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcdump.cpp#L113 & https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcdump.cpp#L1196

However, this one doesn't extra-indent the first line:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcdump.cpp#L755

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

There is no guideline about whitespace as long as you stick to either rule-of-thumb:

Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

ACK

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:get_transaction_docs branch from 51d7868 to 35c7eb0 Feb 3, 2019

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

hmm, ok went with format script if indentation was not intentional. added getmempoolentry reference.

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

utACK 35c7eb0

Show resolved Hide resolved doc/release-notes.md Outdated

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:get_transaction_docs branch from 35c7eb0 to 4701239 Feb 4, 2019

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

This is ready.

@MarcoFalke MarcoFalke requested a review from jnewbery Feb 5, 2019

@jnewbery
Copy link
Member

left a comment

ACK 4701239.

One nit inline, which could be ignored.

blockhash is provided, check the mempool. 3. If no blockhash is
provided but txindex is enabled, also check txindex.
- The `getrawtransaction` RPC & REST endpoints no longer check the
unspent UTXO set for a transaction. The remaining behaviors are as

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 5, 2019

Member

Nit: Perhaps this is fine in American English, but 'The remaining behaviors are...' sounds a little odd to me. 'The new behavior is...' sounds more natural.

This comment has been minimized.

Copy link
@amitiuttarwar

amitiuttarwar Feb 5, 2019

Author Contributor

I disagree, calling the behavior new seems misleading.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 5, 2019

Member

I'll let others comment. ACK either way.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 5, 2019

Merge bitcoin#15332: [Docs] Small updates to getrawtransaction descri…
…ption

4701239 [Docs] Small updates to getrawtransaction description (Amiti Uttarwar)

Pull request description:

  As per review comments on bitcoin#15159

Tree-SHA512: 0bbbe956b47d177f7e67c5ab2048287783327d9e07a679d64d79aee3ea8633e769f75b59d3dbce517924ba5d64d6c44f26bf49e16d40612463e460ad1a238129

@MarcoFalke MarcoFalke merged commit 4701239 into bitcoin:master Feb 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.