Fixes #32698
Problem Inconsistent initialization of class,function, method identifiers and structs. Spread throughout the codebase is screaming / acronym caps.
Solution
JSONRPCRequest should be JsonRpcRequest.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32720.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/43835056566
LLM reason (✨ experimental): The CI failure is caused by a lint check error related to trailing whitespace in a documentation file.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
99@@ -100,6 +100,11 @@ code.
100 naming style](#internal-interface-naming-style) for an exception to this
101 convention.
102
103+ - With `PascalCase`, do not to use [screaming/acronym caps](https://gist.github.com/adashrod/66564c690906c9b774e77ddacbd06e1b). Legacy identifiers that violate this rule remain for compatibility and will be renamed gradually. Hence:
“do not to use” -> “do not use” [remove redundant “to”]
Also, not sure about linking to a private third-party rant as a rationale
...rant.md, also external private third-party content is likely to change or disappear
I think it’s useful to document the reason for the policy (so that any future proposed revisions to the policy understand what the goal was), although I concur with @maflcko that unstable external links are problematic for long-term preservation. I think this could be handled by including a brief summary of the reasoning inline (sufficient to grok what the point of the policy is), perhaps including an external link for attribution purposes.
The actual content of the “rant” doesn’t appear unprofessional to me; it’s pretty much exclusively composed of technically coherent points. I suspect that the word “rant” in the description is intended either in self-deprecating jest, or as a reference to the motivation of writing the document (I assume the author intended to cite this document when criticizing various specific FLOSS projects’ practices on the subject). The document does seem a lot more thorough of a rationale for the policy than I had seen elsewhere (it raised some minor points that I wasn’t aware of when I filed the issue).
I would have no issue with linking to that external document, as long as sufficient summary is included inline that a potential loss of the external link doesn’t cause undue confusion. At the very least, including a link for context is better than providing no context at all: if the link dies, the dead link is unlikely to provide negative value. I think it’s also good-neighbor behavior to attribute documents that influenced the policy; I certainly wouldn’t want people to omit attribution to my work just because they’re worried that the link might become inaccessible in the future.
99@@ -100,6 +100,11 @@ code.
100 naming style](#internal-interface-naming-style) for an exception to this
101 convention.
102
103+ - With `PascalCase`, do not use screaming/acronym caps. Legacy identifiers that violate this rule remain for compatibility and will be renamed gradually. Hence:
0 - With `PascalCase`, Acronyms must be treated as words. To minimize code churn, legacy identifiers that do not comply with this rule will remain unchanged and will only be renamed when the surrounding code is modified. Hence:
NIT: Do not tell what NOT to do but explain what to do. This suggestion explains (imho) what the intention is, treating acronyms as words, and therefor the PascalCase can easily be applied.
To balance attribution with long-term stability, I recommend that we:
Also take into consideration the nit of @janb84.
With these clarifications or adjustments in place, I trust we can all concur on the final structure.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
aaah, forgot. thanks
reACK b6f3b335002a6fa5095bedb8ad22a71fce5d35fb
Changes sinds last ACK: