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

Switch to dep #5

Merged
merged 1 commit into from
Feb 1, 2018
Merged

Switch to dep #5

merged 1 commit into from
Feb 1, 2018

Conversation

adunham-stripe
Copy link
Contributor

This switches to using dep for dependency management, and attempts to reproduce the existing lock.json as faithfully as possible.

@groob
Copy link
Owner

groob commented Feb 1, 2018

@adunham-stripe thanks for adding dep. I think that's a nobrainer as I've moved to dep for almost every other go project.

I'm a bit against checking in /vendor as the lockfile is usually sufficient and adding the vendor folder affects github code review/stats a lot.
Would love to hear your thoughts on the subject.

@adunham-stripe
Copy link
Contributor Author

For libraries, I generally agree that you should not check in vendored code. However, for binaries, I tend to enjoy having everything checked in - it insulates against someone moving/deleting a repository and then being entirely unable to build something. However, I don't have super strong feelings here, so if you'd prefer not, I'm happy to drop the last commit.

@groob
Copy link
Owner

groob commented Feb 1, 2018

Lets do that. I agree with you about the potential for that kind of disaster scenario but from past experience it hasn't been a practical issue. I'd put the option on the table for future updates.

Another question. I tend to prefer 1 commit per pull request and squash on merge. Doing that ensures a clean linear history of the project in master and avoids a lot of the in progress commits common in FOSS PRs. But it does require you to rebase after code review if you want to keep your commits clean.

Would you be ok if I enable that for this repo?

@adunham-stripe
Copy link
Contributor Author

Okay, force-pushed and removed that last commit. As for the squash-and-merge, I don't have super-strong opinions, so that sounds reasonable to me! 👍

@groob
Copy link
Owner

groob commented Feb 1, 2018

Perfect. I'll try to put out a contributing guide and add CI to this project shortly. Thanks for the PR!

@groob groob merged commit 3ed7d42 into groob:master Feb 1, 2018
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