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

sum-of-multiples: Implemented issue #141 #147

Closed
wants to merge 1 commit into from
Closed

sum-of-multiples: Implemented issue #141 #147

wants to merge 1 commit into from

Conversation

Dog
Copy link
Contributor

@Dog Dog commented Nov 8, 2014

Implemented issue #141 which requested that sum_of_multiples use a function rather than a class.

I implemented the multiples as a list because it made it easier to differentiate them from the limit when looking at the test file. In the example I set the default multiples to None to avoid a python gotcha with mutable default arguments.

http://docs.python-guide.org/en/latest/writing/gotchas/

@Dog Dog changed the title sum_of_multiples: Implemented issue #141 sum-of-multiples: Implemented issue #141 Nov 8, 2014
@Dog
Copy link
Contributor Author

Dog commented Nov 18, 2014

Is there an issue with this implementation?

@kytrinyx
Copy link
Member

I think this just slipped through the cracks. Thanks for pinging!

@sjakobi would you sanity-check this when you have a moment?

@sjakobi
Copy link
Contributor

sjakobi commented Nov 18, 2014

Sorry for the delay, @Dog!

I wanted to merge both your changes and #139 but it seemed complicated and at some point I forgot about it.

@kytrinyx: Do you see a way to merge both PRs and resolve the conflicts between them?

@kytrinyx
Copy link
Member

I generally merge things locally when there are conflicts.

I would suggest merging #139 and then you can do this locally:

git checkout master && git fetch origin master && git reset --hard master && git fetch origin refs/pulls/139/head && git checkout -b Dog/sum-of-multiples FETCH_HEAD

which will make sure that you have a fresh master as well as a local branch with the pull request in it.

Then you can run git rebase master on the Dog/sum-of-multiples branch to resolve conflicts per usual. @Dog's commits will still be attributed to him, and you can merge onto master locally once the commits are resolved.

The other option is to merge one and ask the other author to rebase onto the new master and resolve commits :) I will generally go this route if I'm unsure of how I would resolve the conflicts.

@Dog
Copy link
Contributor Author

Dog commented Nov 18, 2014

No problem. I had a feeling if there was an issue someone would have commented. I'm still getting accustomed to working on Github though.

Thanks

@kytrinyx
Copy link
Member

This is closed by #149, I think. If I'm wrong, please reopen this!

@kytrinyx kytrinyx closed this Nov 24, 2014
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.

3 participants