[tor-bugs] #10706 [Tor Weather]: Find out how to integrate new Python scripts into current Weather

[Tor Bug Tracker & Wiki]

#10706: Find out how to integrate new Python scripts into current Weather
-----------------------------+-----------------------------
     Reporter:  karsten      |      Owner:  feverDream
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:
    Component:  Tor Weather  |    Version:
   Resolution:               |   Keywords:  weather-rewrite
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+-----------------------------
Changes (by karsten):

 * status:  new => needs_revision


Comment:

 feverDream, you asked me to review [https://github.com/achintangal
 /weather-rewrite/commit/5e533db1e9d02b13848d308b5db61b752529d60d this
 commit] related to this ticket.  Here's some feedback:

  - My first observation is that you have quite a few commits in that
 branch, but they are not related to adding a single feature or fixing a
 single bug.  That means your branch cannot be merged easily.  Good thing
 we're using Git!  For this ticket, please create a separate branch which
 only contains the commit(s) necessary to implement the feature.  You may
 want to cherry-pick, squash, and split commits until you come up with a
 commit history you think is easy to review.  Then send a "pull request" by
 adding a link to that specific branch to this ticket.  In the following,
 I'll comment only on the single commit you mentioned.
  - Rather than importing `sqlite3` and writing SQL code ourselves, is
 there a way to access the database by means of Django's abstractions?
 What if we want to switch to a different database later?
  - You're using quite different styles of comments: `#comment`, `#
 comment` (yes, whitespace counts), `## comment`, `## COMMENT ##`, `#
 commented-out code` (which should simply be removed, not left in), `code #
 comment`, `"""\ncomment\n"""` (inside of functions, not at function
 start).  Can you check what comment styles are currently used in Weather
 and adapt your comments?
  - What does the name `OnooUtil` stand for?  Would `OnionooWrapper` be a
 more accurate class name?
  - How does `OnooUtil` use a bandwidth document for answering the various
 things like whether a relay is up or hibernating?
  - Actually, `is_up_or_hibernating` doesn't do what it promises to do.
  - And finally, how is this code integrated into the current Weather?  Who
 calls it and when?  Does this require an update of the `README` or
 `doc/INSTALL`?

 Sorry for being picky, but this commit needs more work before being
 merged.  Happy to do another review when you have another revision.
 Thanks!

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