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

#93 - Adding support for scan.all() #140

Merged
merged 10 commits into from
May 15, 2017
Merged

#93 - Adding support for scan.all() #140

merged 10 commits into from
May 15, 2017

Conversation

fishcharlie
Copy link
Member

This pull request attempts to add support for scan.all() as described in #93. I tried to integrate this with as minimal code changes as possible but there might be a better way to integrate this more cleanly and with less disruption to the code base.

Feel free to discuss and let me know if there is anything else I can do to get this merged in.

@brandongoode
Copy link
Contributor

Thanks for the PR. It's a great addition. Give me a few days to look it over.

@fishcharlie
Copy link
Member Author

Thanks Brandon. Let me know if I can make any adjustments or updates.

I'm not sure why the tests are failing. I ran the tests locally and they all passed.

I also just realized if for whatever reason options.all gets overwritten to null that will cause problems.

I'm currently on my phone but in a little bit I'll try to look into both isssues and repush if I'm able to come up with fixes.

@fishcharlie
Copy link
Member Author

@brandongoode That should fix the undefined issue. Still not sure why tests are failing. Any insight into this would be great. Let me know what other changes or adjustments you need from me when you have a chance to look it over.

@brandongoode
Copy link
Contributor

@fishcharlie, It looks good. The failure was just a timing issue that happens with TravisCI. I reran the test, and it's passing now.

Could you add the feature to the Scan section in docs/index.md and some test cases?

@fishcharlie
Copy link
Member Author

@brandongoode Just added the documentation. Tried to add 2 unit tests. I'm not the best/most experienced with unit tests. So not sure if they are good tests or anything like that. If someone else wants to write better unit tests feel free since I'm not the best at that haha.

@brandongoode
Copy link
Contributor

Thanks. I'll pull it down and add a few more checks to the test cases.

@brandongoode brandongoode merged commit 925ae31 into dynamoose:master May 15, 2017
@fishcharlie fishcharlie deleted the issue#93 branch May 15, 2017 21:07
@brandongoode brandongoode added this to the v0.8.0 milestone Jun 1, 2017
@brandongoode brandongoode mentioned this pull request Jun 1, 2017
4 tasks
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.

2 participants