move pages to new website style #45

Closed
wants to merge 7 commits into from

2 participants

@billsaysthis

No description provided.

@geoelectric geoelectric and 1 other commented on an outdated diff Nov 29, 2011
rules = memcache.get("rules")
if(rules is None):
rules = urlfetch.fetch("http://wiki.hackerdojo.com/api_v2/op/GetPage/page/Event+Policies/_type/html", "GET").content
- memcache.add("rules", rules, 86400)

Why not add the rules to the cache?

I believe you are correct and this line should not have been deleted. Can you merge my code and then add back the line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@geoelectric

Other than wondering about caching the rules, I see no problems. If you can clarify that, I'll merge.

@billsaysthis
@billsaysthis
@geoelectric

Dude. :)

Re: replacing them, not sure we're talking about the same thing since I mean a bit that got taken out in this commit.

Take a quick look at the line in main.py I'd marked w/ an inline comment (451 in the original, gone in the new).

It takes out adding the rules page to memcache when we retrieve it, but the rest of the cache logic is still there as dead code. There's a discrepancy there--either we should be using cache or removing the code that checks cache first.

@billsaysthis

Geo, if you think the lines are unnecessary/wrong I defer to your judgment since I thought I'd mistakenly removed them and don't really understand exactly why we'd have or not have them.

@geoelectric

I'm not trying to be frustrating here, but you seem to be answering a different question than what I'm asking. I'm not referring to anything you added.

You -removed- a line of logic, and I'm asking if you meant to remove it. Line's referenced above.

I can't defer to my judgment on this because I'm not saying it's wrong; I'm asking why line 451 of main.py (add rules to cache) was removed, or whether that was a checkin/merge mistake of some kind.

@billsaysthis

What version of main.py had this line? I went back and looked at the old versions but don't see it.

IAC I think Brian needs to be asked, or Jeff as I don't know what you're talking about if we are thinking of different questions.

@billsaysthis

I will say that my local instance shows the correct result (i.e., using the version of code I checked in).

@geoelectric

I'm looking directly at the diff in your Pull Request, which is where I figured you'd be looking too since that's what I'm reviewing. :)

https://github.com/hackerdojo/hd-events/pull/45/files#L0R458

Second line from the top. It's the red one with the minus.

In current code, that line is here:

https://github.com/hackerdojo/hd-events/blob/master/main.py#L451

Can't comment on your local version. Doesn't matter, either. If I merge what you've given me, we'll turn off caching on rules, per the diff.

@billsaysthis

Did we not settle this in email, that you will make this merge and then put the cache line back?

@geoelectric
@billsaysthis
@billsaysthis

Dude, still nothing?

@geoelectric

You're right.

I should have never agreed to do the fix for you, since I'm plainly not going to find time to dick around, add your github as a remote, download your branch, make your fix, add hackerdojo as a remote, merge it, and push. I only had time to do the 30s automerge I agreed to do in the first place when you got your own damned patch right.

You're an owner on the Hacker Dojo repo. Fix it and land it yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment