Skip to content

Conversation

amitiuttarwar
Copy link
Contributor

@amitiuttarwar amitiuttarwar commented Feb 3, 2019

As per review comments on #15159

@fanquake fanquake added the Docs label Feb 3, 2019
@amitiuttarwar
Copy link
Contributor Author

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

@maflcko
Copy link
Member

maflcko commented Feb 3, 2019

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

@maflcko
Copy link
Member

maflcko commented Feb 3, 2019

ACK

@amitiuttarwar
Copy link
Contributor Author

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

@kristapsk
Copy link
Contributor

utACK 35c7eb0

@amitiuttarwar
Copy link
Contributor Author

This is ready.

@maflcko maflcko requested a review from jnewbery February 5, 2019 19:54
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree, calling the behavior new seems misleading.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll let others comment. ACK either way.

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Feb 5, 2019
…ption

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

Pull request description:

  As per review comments on bitcoin#15159

Tree-SHA512: 0bbbe956b47d177f7e67c5ab2048287783327d9e07a679d64d79aee3ea8633e769f75b59d3dbce517924ba5d64d6c44f26bf49e16d40612463e460ad1a238129
@maflcko maflcko merged commit 4701239 into bitcoin:master Feb 5, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants