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

Issue38fix #43

Merged
merged 6 commits into from
Jan 1, 2014
Merged

Issue38fix #43

merged 6 commits into from
Jan 1, 2014

Conversation

MehdiK
Copy link
Member

@MehdiK MehdiK commented Dec 31, 2013

@robdmoore @hazzik could you please review? Thanks

This was referenced Dec 31, 2013
@robdmoore
Copy link
Contributor

Nice! I like how you have made the new code "safer" by allowing people to opt-in. Very cool.

@robdmoore
Copy link
Contributor

re: inline data I'm happy to put in another PR since I think I know how to do it

@MehdiK
Copy link
Member Author

MehdiK commented Dec 31, 2013

Cool. I hate the for-loop data injection for tests but also hate to repeat all those entries 2500 times! Looking forward to your PR.

@robdmoore
Copy link
Contributor

I'll do it after you merge this one to avoid conflicts

@robdmoore
Copy link
Contributor

This looks good to me - added a few minor comments

@MehdiK
Copy link
Member Author

MehdiK commented Dec 31, 2013

Thanks mate. I fixed the issues you raised.

MehdiK added a commit that referenced this pull request Jan 1, 2014
@MehdiK MehdiK merged commit b473f0b into master Jan 1, 2014
@MehdiK MehdiK deleted the issue38fix branch January 1, 2014 06:09
@MehdiK
Copy link
Member Author

MehdiK commented Jan 1, 2014

@hazzik Although this is merged, I would still really appreciate your review and will capture your notes as issues to be resolved in future changes.

@hazzik
Copy link
Member

hazzik commented Jan 1, 2014

@MehdiK looks good for me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants