Skip to content

Conversation

dvrkps
Copy link
Contributor

@dvrkps dvrkps commented Oct 5, 2017

No description provided.

@tleen
Copy link
Member

tleen commented Oct 6, 2017

Thanks for the contrib @dvrkps! The extra files included in the commit are a side-effect we are seeing from having the master and nextercism branches. They should not be in this commit, so see how we figure it out in #862.

Also while the nextercism travis file is outdated, the master one is actually ahead of this commit:

go:
  - 1.8.3
  - 1.9
  - tip

I wonder if we should be merging/rebasing nextercism against master periodically? @robphoenix thoughts?

@ferhatelmas
Copy link
Contributor

@tleen you didn't ask me but I'd say definitely.

Copy link
Contributor

@ferhatelmas ferhatelmas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me but it's better to base PR into master, IMHO.

- 1.7.5
- 1.8
- 1.8.x
- 1.x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

# but anyway, it could happen and it wouldn't be our problem.
matrix:
include:
# Keep this in sync with the latest version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe comment can be removed. 2.x isn't close.

@tleen
Copy link
Member

tleen commented Oct 6, 2017

@tleen you didn't ask me but I'd say definitely.

😳 Apologies @ferhatelmas ! I didn't want to spam people with mentions, I'll hit the whole group next time.

@dvrkps
Copy link
Contributor Author

dvrkps commented Oct 7, 2017

Can I add this in master branch also ?

@robphoenix robphoenix changed the base branch from nextercism to master October 9, 2017 16:15
Copy link
Contributor

@robphoenix robphoenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to be merged into the master branch.

This is good to merge with the suggested line deletion from @ferhatelmas

Thanks @dvrkps! I didn't know you could do this with Travis, learnt something 👍

@robphoenix
Copy link
Contributor

robphoenix commented Oct 9, 2017

I wonder if we should be merging/rebasing nextercism against master periodically? @robphoenix thoughts?

Yeah, for sure.

I've just rebased off master and pushed up to nextercism.

@robphoenix
Copy link
Contributor

I've just rebased off master and pushed up to nextercism.

I haven't. I thought I had, but I had just pushed to my own repo origin rather than this one upstream.

If one of us rebases nextercism on master...

git fetch upstream master
git rebase upstream/master

...then they would have to force push up to this repo. This can have any number of negative consequences, including attaching that person to many of the commits

Honestly, I'm not sure of the best way to keep the nextercism branch in sync with master.

I'm going to merge this in to master, as the only change requested is not to do with something introduced in this PR and can be dealt with separately. And any conversation concerning branch maintenance can go in a separate issue, rather than spamming this PR.

Thanks again @dvrkps! 🎉

@robphoenix robphoenix merged commit d44b036 into exercism:master Oct 10, 2017
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.

4 participants