[tor-bugs] #31794 [Circumvention/Snowflake]: Errors swallowed

[Tor Bug Tracker & Wiki]

#31794: Errors swallowed
-------------------------------------+------------------------------
 Reporter:  sah                      |          Owner:  (none)
     Type:  defect                   |         Status:  needs_review
 Priority:  Medium                   |      Milestone:
Component:  Circumvention/Snowflake  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:                           |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:  cohosh                   |        Sponsor:
-------------------------------------+------------------------------

Comment (by sah):

 Hi dcf, thanks for taking the time to review the ticket.

 WRT Error strings.
 I refer you to the widely cited
 https://github.com/golang/go/wiki/CodeReviewComments#error-strings
 Error strings should not be capitalized (unless beginning with proper
 nouns or acronyms) or end with punctuation, since they are usually printed
 following other context. That is, use fmt.Errorf("something bad") not
 fmt.Errorf("Something bad"), so that log.Printf("Reading %s: %v",
 filename, err) formats without a spurious capital letter mid-message. This
 does not apply to logging, which is implicitly line-oriented and not
 combined inside other messages.

 I opted to exempt these instances from the rule for two reasons.
 Firstly: They are succinct, and it's absolutely clear to the user
 receiving the error what's wrong, and where the problem is.
 Secondly: It's also clear that they are temporary, that is, the error
 message will be removed once the functions/methods are implemented.

 Where I have changed them, I don't think I should have and will go back
 and reset them.


 With respect to the explicit nil value. It's unnecessary to have that code
 exist, one of the features of Go is that no variable is uninitialised. I
 don't think that the explicitness provides any advantage for the
 maintainer, because it assumes that they do not know how to code in Go.

 WRT shadowed err:
 The confusion caused by the shadowing is evident in your comment, was it
 deliberate or not? When there is a desire for the inner scoped error to be
 separate and distinct from the outer scoped error then a separate and
 distinct identifier is appropriate. (Curiously this is the reverse of the
 explicit setting of the nil value for the variables mentioned in the
 previous paragraph)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31794#comment:14>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online