Opened 7 years ago

Closed 4 years ago

#10706 closed enhancement (wontfix)

Find out how to integrate new Python scripts into current Weather

Reported by: karsten Owned by: feverDream
Priority: Medium Milestone:
Component: Metrics/Tor Weather Version:
Severity: Keywords: weather-rewrite
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Find out how we'd integrate the daily script and later the hourly script into existing Weather and how these scripts would talk back to Weather to send out emails and talk to the database. Assigned to AC on January 15.

Child Tickets

Change History (4)

comment:1 Changed 7 years ago by karsten

Some notes from yesterday's meeting:

comment:2 Changed 7 years ago by karsten

Status: newneeds_revision

feverDream, you asked me to review 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!

comment:3 Changed 7 years ago by feverDream

Thanks for suggestions karsten. With regard to the git, I will makes the necessary changes.

About the integration code:

  1. sqlite3 -> django's db: I am planning to fix it once I get everything in order. Right now, I use db-code from Olivers hourly script to get the job done.
  2. You are right, the is_up_or_hibernating function doesn't really check anything. fixed it.
  3. About the name of the wrapper class, I didn't really give it much thought, but OnionooWrapper makes sense.

Lastly, the work-flow is more or less the same with the current weather - a django background command(via cron) calls the get_documents() function in the "" script, which grabs the new document and caches it, or uses the cached document if new document is not present.

It then initializes a new OninooUtil/OnionooWrapper object with the document and passes it to the updaters.run_all() function. Which then uses the wrapper object to update the routers table, check subscriptions and send emails.

Last edited 7 years ago by feverDream (previous) (diff)

comment:4 Changed 4 years ago by karsten

Resolution: wontfix
Status: needs_revisionclosed

Tor Weather has been discontinued as of May 24, 2016: Batch-closing all remaining tickets as announced in #19382. A list of these tickets and any other Weather tickets modified after June 26, 2016 will be available here:^Metrics%2FTor+Weather

Note: See TracTickets for help on using tickets.