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

A few ideas #2

Closed
mattfarina opened this issue Mar 4, 2016 · 5 comments
Closed

A few ideas #2

mattfarina opened this issue Mar 4, 2016 · 5 comments
Milestone

Comments

@mattfarina
Copy link

Thanks for putting this together. I know it will be useful.

I have a few thoughts that may help you along.

  1. Don't strip non-Go files by default. Instead opt-in to that with a flag. The reason is those documents can contain license information. Sometimes it's in a license file. Sometimes it's in a readme. You never can be sure.

A good default for licenses is important. Take the BSD license. If someone removes the license and someone redistributes the parent project on GitHub that person is in violation of the BSD license. There are a bunch of things like that.

Would you be open to opting in on that rather than opting out? That way those who can do it, such as a proprietary codebase, can opt-in. It spares accidents for open source projects.

  1. This project deletes things. So, can you add some testing in? Maybe a testdata directory and some tests. Maybe even cross platform (POSIX and Windows)? If you're looking for an example you can see what I did on rmvcsdir. I'm happy to talk more about this.

It's reassuring to know the tests make sure it works right and for those looking to use it.

  1. If you're not going to keep the dependencies in the vendor/ folder, consider adding vendor/ to the .gitignore file. I was considering a pull request but I ran out of time when I was in my dev environment.

  2. This is entirely optional but, glide has a plugin system like git. If this were named glide-vc (or something else) you could use the command glide vc when the plugin is installed. Just a thought. I'm not all that opinionated on this. I just like to raise awareness.

Nice work. I'm happy to see you working on this.

@sgotti
Copy link
Owner

sgotti commented Mar 5, 2016

@mattfarina Thanks to you!

I just putted it online as is for review, so everything can be changed.

Before committing I'll prefer to open some PRs to keep a good review workflow.

Would you be open to opting in on that rather than opting out? That way those who can do it, such as a proprietary codebase, can opt-in. It spares accidents for open source projects.

Looks good. So by default keep all the files inside needed packages, so what do you think about these additional options (instead the current keep-.*)?

--only-go (remove all but go files)
--no-tests (remove also go test files, requires --only-go) 

Perhaps you have in mind a better logic, names and wording on these options.

  1. This project deletes things. So, can you add some testing in?

Sure! I just wanted to put it up (usually I always write tests :D)

  1. If you're not going to keep the dependencies in the vendor/ folder, consider adding vendor/ to the .gitignore file. I was considering a pull request but I ran out of time when I was in my dev environment.

Yeah. I wasn't sure if it's better to not commit or commit vendor/ (stripped or not stripped with gvc? I have to look at the licenses 😄) is worth. What do you think?

  1. This is entirely optional but, glide has a plugin system like git. If this were named glide-vc (or something else) you could use the command glide vc when the plugin is installed. Just a thought. I'm not all that opinionated on this. I just like to raise awareness.

I thought about this, but then forgot when choosing the name 😞 . I think that, to make it go installable with the glide-vc name, I should rename this repository to glide-vc. What do you think?

@mattfarina
Copy link
Author

@sgotti sorry I've not been more responsive. Been a little busy. I like where you're taking this.

In Glide 0.10 we're going to add the base ability to strip VCS directories from the vendor directory. That's the only part of this. For those who would like to prune further I'd like to list this project as a method to do that. I'm expecting the release of 0.10 to be a couple weeks away.

@sgotti
Copy link
Owner

sgotti commented Mar 31, 2016

@mattfarina No problem. I think I fixed many of your suggestions (just need to handle the glide-vc vendoring situation 😄 )

Glide 0.10 is really cool, thanks for listening to my suggestions and improving it. I also tried to explain my current workflow (personal and very aggressive, but similar to the current godeps workflow) to the coreos guys that are looking to use glide (containers/build#186 (comment) coreos/docs#775).

@mattfarina
Copy link
Author

@sgotti thanks for pointing me to those issues. I'll take a look there. If you want to pull me into a coreos or appc conversation on the topic please feel free. I'm even happy to jump on a teleconference or other chat if needed.

If you're happy with what we've already talked about here feel free to close this issue. I don't want it to sit around and linger. I'll leave closing it for when you're ready. Otherwise I'm good and I'm glad you've written this.

@sgotti
Copy link
Owner

sgotti commented Sep 24, 2016

I think this can be closed. Thanks!

@sgotti sgotti closed this as completed Sep 24, 2016
@sgotti sgotti modified the milestone: v0.1.0 Sep 25, 2016
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

No branches or pull requests

2 participants