Page MenuHomePhabricator
Differential D1298

Update developer notes and link to it from contributing docs
ClosedPublic
Actions

Authored by schancel on Apr 19 2018, 03:27.

Details

Reviewers
deadalnix
jasonbcox
mbhinder
jimpo
matiu
Mengerian
Group Reviewers
Restricted Project
Commits
rSTAGING81c8fef18aeb: Update developer notes and link to it from contributing docs
rABC81c8fef18aeb: Update developer notes and link to it from contributing docs
Summary

As per title.

Test Plan

Read the documentatino

Diff Detail

Repository
rABC Bitcoin ABC
Branch
style-guide
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2351
Build 2837: Bitcoin ABC Buildbot (legacy)
Build 2836: arc lint + arc unit

Event Timeline

schancel created this revision.Apr 19 2018, 03:27
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 19 2018, 03:27
teamcity added a subscriber: teamcity.Apr 19 2018, 03:29

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Apr 19 2018, 03:52

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2350: Diff 3549.Apr 19 2018, 03:52
matiu added a comment.Apr 19 2018, 13:08

LGTM

schancel updated this revision to Diff 3550.Apr 19 2018, 14:17

Add class doxygen comment in example.

teamcity added a comment.Apr 19 2018, 14:19

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Apr 19 2018, 14:42

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2351: Diff 3550.Apr 19 2018, 14:42
jasonbcox requested changes to this revision.Apr 19 2018, 16:34
jasonbcox added inline comments.
doc/developer-notes.md
28

Curly braces should be on the same line as the definition. Same goes for lines below this one.

53

Rather than what this code does, perhaps "Comment summarizing the intended purpose of this section of code". Unless the code is particularly difficult to parse, describing what it does is a bit redundant. The intended purpose is often very similar to what it does, but we all know that isn't always the case. :P

324

Should we leave this in until T214 is completed?

This revision now requires changes to proceed.Apr 19 2018, 16:34
jimpo added inline comments.Apr 19 2018, 21:14
doc/developer-notes.md
18

Remove the allowance for single-line if statements.

39

Presumably lowerCamelCase for local variables too? Can we document that somewhere?

44

Can you make it explicit that this applies to both functions and class methods?

56

Braces.

schancel updated this revision to Diff 3553.Apr 20 2018, 01:29

Update based on feedback, and include some additional suggestions

teamcity added a comment.Apr 20 2018, 01:29

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

schancel updated this revision to Diff 3554.Apr 20 2018, 01:35

Delete bad examples of comments, fix a function name.

teamcity added a comment.Apr 20 2018, 01:37

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Harbormaster completed remote builds in B2354: Diff 3553.Apr 20 2018, 01:48
teamcity added a comment.Apr 20 2018, 01:48

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

teamcity added a comment.Apr 20 2018, 01:59

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2355: Diff 3554.Apr 20 2018, 01:59
jimpo accepted this revision as: jimpo.Apr 20 2018, 03:00
jasonbcox added a comment.Apr 20 2018, 22:02

One last clarification on block statements and I think it's good. :)

doc/developer-notes.md
18 ↗(On Diff #3554)

These are called "block statements". We can clarify this by saying:

  • Always add braces for block statements (if, for, while, etc.).
schancel updated this revision to Diff 3574.Apr 23 2018, 21:03

Update block statement text

teamcity added a comment.Apr 23 2018, 21:05

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Apr 23 2018, 21:21

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Apr 23 2018, 21:40

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2369: Diff 3574.Apr 23 2018, 21:40
jasonbcox accepted this revision.Apr 23 2018, 22:01
This revision is now accepted and ready to land.Apr 23 2018, 22:01
Closed by commit rABC81c8fef18aeb: Update developer notes and link to it from contributing docs (authored by schancel). · Explain WhyApr 23 2018, 22:32
This revision was automatically updated to reflect the committed changes.
deadalnix added inline comments.Apr 23 2018, 22:43
doc/developer-notes.md
278 ↗(On Diff #3575)

Why was this removed ? This is good advice and should be adapted. Plus it has nothing to do with style guide so has nothing to do in that diff anyway.

359 ↗(On Diff #3575)

dito

372 ↗(On Diff #3575)

dito

deadalnix added inline comments.Apr 23 2018, 22:52
doc/developer-notes.md
27 ↗(On Diff #3575)

This isn't a formating rule. While this is a good rule, it has nothing to do burried in the middle of formatting rules.

Revision Contents
Changeset List

PathSize
1 line
doc/
79 lines

Diff 3550

View Options

CONTRIBUTING.md

Loading...
View Options

doc/developer-notes.md

Loading...