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

Async Functionality & Object-Oriented Representation #14

Merged
merged 14 commits into from May 13, 2018

Conversation

robertjkeck2
Copy link
Contributor

  1. Implements aiohttp and asyncio in a very simple manner to allow for asynchronous operations when faced with multiple Item IDs.
  2. Gives the user the ability to expand Item IDs into Item objects when using the get_item or get_user methods
  3. Gives the user the ability to see raw JSON representation of the *_stories methods instead of the Item object representation.
  4. Gives the user the ability to filter by item_type when using the get_items_by_ids method

@robertjkeck2 robertjkeck2 reopened this Apr 26, 2018
@robertjkeck2
Copy link
Contributor Author

Looks like the async functions aren't working in the Travis build. Any ideas here?

@avinassh
Copy link
Owner

hey, thank you very much for the PR. I am extremely busy with my work, so please give me a day and I will review this over the weekend

😅

@coveralls
Copy link
Collaborator

coveralls commented Apr 26, 2018

Coverage Status

Coverage increased (+10.4%) to 98.824% when pulling f129a66 on robertjkeck2:async into a1b039d on avinassh:master.

@robertjkeck2
Copy link
Contributor Author

@avinassh I appreciate it. Some thoughts: Using asyncio drops coverage for python2. Using f strings drops everything but python3.6. Is that ok with you or do you want to try to keep backwards compatibility? Also, I got rid of the get_all endpoint because it takes a very long time to run AND it's probably irresponsible to scrape all of HN with the size it is now. Thoughts?

@robertjkeck2
Copy link
Contributor Author

Also, @avinassh sorry for the multiple TravisCI builds. I was having trouble setting up a dev environment that worked with async. Should be good to go with the updated .travis.yml now.

@avinassh
Copy link
Owner

We will do a new release with major version number and I don't really mind losing python 2 support. I am fine with using F strings too.

I would still keep get_all, since a lot of people use HN data to analyse etc.

As far travis builds, its okay! feel free to experiment!

@robertjkeck2
Copy link
Contributor Author

@avinassh when testing the get_all function, I always get a Killed: 9 error due to the in-process data filling up memory on my laptop. Curious to see if you get the same result. Have you had the time to run through the new code yet?

@avinassh
Copy link
Owner

avinassh commented May 9, 2018

@robertjkeck2 I haven't.

Also, can you let me know by bumping this thread when PR is ready?

if everything works except get_all, then ignore that. I will try to debug. In the other branch I was working on, I did not store child comments in the memory, may be thats why its running out of the memory?

@robertjkeck2
Copy link
Contributor Author

@avinassh PR is ready. We can deal with the get_all function in a future PR since this one is good to go (and should work with enough RAM).

@avinassh
Copy link
Owner

Looks fine to me. Merging and making a major release

@avinassh avinassh merged commit 658ac14 into avinassh:master May 13, 2018
@robertjkeck2 robertjkeck2 deleted the async branch May 14, 2018 15:16
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

3 participants