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

LIST(impl): Add map implementation #51

Merged
merged 2 commits into from
Jul 30, 2017
Merged

LIST(impl): Add map implementation #51

merged 2 commits into from
Jul 30, 2017

Conversation

SimonMeskens
Copy link
Contributor

Can I get a code review to see if these are the kinds of PRs you want? I'm versed in rebase if you need changes to the commit structure too.

@axefrog
Copy link
Member

axefrog commented Jul 30, 2017

Sorry for the slow reply (Sunday and whatnot). Going to review this now. On a side note, do you by any chance have Docker set up on your local machine? Travis is failing and nobody I know can seem to reproduce the error that Travis is reporting. See: https://travis-ci.org/frptools/collectable/jobs/259185345
Apparently if you run the Travis test instance in Docker, it'll reproduce the issue successfully. Unfortunately my machine seems to fail when installing prerequisites for Docker, so I haven't been able to figure the issue out yet, which means that pull requests always show up as build failures.

@axefrog axefrog merged commit 0e2c4f0 into frptools:master Jul 30, 2017
@axefrog
Copy link
Member

axefrog commented Jul 30, 2017

Code's all good, and builds fine for me locally. Merged. Thanks!

@SimonMeskens
Copy link
Contributor Author

Unfortunately, Docker doesn't want to install on my machine either and I haven't been able to find out why.

Thanks for the review, I'll move on to some more challenging PRs (wanted to make sure I understood the PR process first).

@axefrog
Copy link
Member

axefrog commented Jul 31, 2017

Travis is finally building. Your future PRs should now actually reflect this.

@SimonMeskens
Copy link
Contributor Author

Thanks, that's helpful

@SimonMeskens SimonMeskens mentioned this pull request Oct 31, 2017
15 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