Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some code to take a look at #1

Merged
13 commits merged into from
Sep 3, 2010
Merged

Some code to take a look at #1

13 commits merged into from
Sep 3, 2010

Conversation

Julian
Copy link
Contributor

@Julian Julian commented Sep 3, 2010

Hey!

Did a pretty small amount of work on your code. I've been meaning to do a Reddit API binding myself for a while but have never found the time, so it's great that you've started it, I'd love to help if I can.

Take a look, see if what I've got interests you if you'd like. I'll be hopefully doing some more work if I get a chance. I haven't tested much other than quickly checking that I haven't broken anything you had already in an obvious way, so keep that in mind I guess as you look over the code.

Some things that I haven't fixed yet are, for example, a lot of the methods that take a parameter @limit@ will hang if given limit as a string, so we should probably be int()'ing what comes in there. The problem is that there are those parameters all over the place, so we'd need to factor a bit. There are a few other things I have here too, so I'll either get to them or you will.

Get in touch with me if you'd like, I'd love to discuss whatever with you.

Thanks!

MY added 11 commits September 2, 2010 19:28
…use a rename). Instead of sleeping after every call (which I assume was temporary), it should just wait after anything called within the REDDIT_API_WAIT_TIME from the last call now. This breaks functionality though, since the lambdas are blowing up. Fix coming.
…ubclasses. Changed __repr__'s on subclasses to __str__.
…__repr__. Now raises NotImplementedError if the subclass doesn't define __str__ (which needs to be defined in order to know what to use for the second position). Alternatively we could just return an empty string, but this is probably nicer.
… I don't think, we could just pass it off to the API to check... But in the meantime they're probably better here than as top level constants.
@tmelz
Copy link
Contributor

tmelz commented Sep 3, 2010

Hey man, nice to see some further work on the project!

Yeah, I definitely didn't implement any checking of user input like the 'limit' or the 'sort' parameters. I'd definitely like to get that polished up: your help would be much appreciated.

My main goal for the project ATM is to document all of the API features and implement them (notably friending, getting more comments, etc.). If you google 'reddit is fun api', you'll see that there has been some documentation of the API, but it's not very approachable, IMHO. Getting some docs up, then adding them to the Reddit API code page would be great.

I actually just started school and I'm swamped with work right now, but tomorrow afternoon I will checkout your commits a bit more thoroughly before I merge them (most definitely will, though). My personal email is timothy.mellor@gmail.com, and I'm on gtalk fairly frequently; feel free to chat me up.

Rock on!

MY added 2 commits September 2, 2010 22:39
… conditions, but it's a tiny bit more concise, and a bit easier to maintain.
@Julian
Copy link
Contributor Author

Julian commented Sep 3, 2010

Yeah, I personally haven't taken any serious look at trying to understand the API yet. I'll take a look hopefully, see if I can be of some more help. So far whatever I've got was just basic functionality stuff based on modifying what you had already. I'll take a look at the API though like you said, as yeah that's probably a much higher priority.

I can relate on being swamped, I happen to be pretty tied up myself, actually tonight's my night off and I used it to do some more coding :). We'll be in touch, I'll push some more of these commits out, take a look at them at your leisure.

Cheers!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants