Skip to content

Add go.mod to support modules landed in go1.11 #264

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

Merged
merged 1 commit into from
Sep 9, 2018
Merged

Add go.mod to support modules landed in go1.11 #264

merged 1 commit into from
Sep 9, 2018

Conversation

antham
Copy link
Contributor

@antham antham commented Sep 5, 2018

Fix #243

@codecov-io
Copy link

codecov-io commented Sep 5, 2018

Codecov Report

Merging #264 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #264   +/-   ##
=======================================
  Coverage   84.82%   84.82%           
=======================================
  Files           9        9           
  Lines        1364     1364           
=======================================
  Hits         1157     1157           
  Misses        206      206           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba7e6ad...0d7a697. Read the comment docs.

@antham
Copy link
Contributor Author

antham commented Sep 5, 2018

This solve the bug, but if go.mod is added, it has to be updated every time dep is updated @jesseduffield

@dawidd6
Copy link
Collaborator

dawidd6 commented Sep 5, 2018

Nice job

@jesseduffield
Copy link
Owner

I have a few questions:

  1. Is there a real need to support modules given that they have been deemed experimental in the release notes? Reading trashhalo's recent comment here it appears that vgo has several issues due to its infancy, and given that I've had no problems so far using dep, I don't see a strong compelling reason to support vgo right now.

  2. If we want to support vgo, is that something we can do simultaneously while using dep? Do we need to pick one or the other?

  3. How do we go about updating dependencies using vgo? With dep, the only thing I've really ever needed to do is
    dep ensure whenever a new package is added, and
    dep ensure -update github.com/jesseduffield/gocui whenever I need to pull in changes from my gocui fork (or any other fork)
    That is a very simple workflow for our current use cases. If vgo proves more of a hassle, I'm not seeing a good reason to make the switch. Furthermore the docs here look extremely involved, and seem to mandate semantic versioning on dependencies, but perhaps I'm misreading that

@antham
Copy link
Contributor Author

antham commented Sep 6, 2018

My idea here was to solve this issue and adding module support, I didn't think to switch from dep. Yes we can have both. What I was thinking to have no difference with dep and not having a complicated flow is simply, at the moment to update dep files when you need, delete go module files and redo a init. When you do a init with a dep file, it will use it to create the go mod file. And you can have both yes.

@jesseduffield
Copy link
Owner

jesseduffield commented Sep 6, 2018

Fair enough. I don't really consider the original issue to be a bug because it's a matter of a user enabling an experimental feature and then an issue occurring. But if there's minimal overhead I suppose there's no harm (though if we find out it's causing issues I'll just revert to only using dep)

Is there some way that we can confirm in CI that a go-get on the repo is possible without it crashing as it does in the original issue? i.e. if export GO111MODULE=on is called?

Otherwise there's a good chance that we'll forget to do the init and we won't have the two dependency files in-sync.

@antham
Copy link
Contributor Author

antham commented Sep 7, 2018

It's still a bug, I guess more and more people will enable modules (as it's going to be the standard) over time and it will crash when they will go get, just an assumption.

Yep it's quite easy to have the CI checking something about it but if you don't look at what the CI report you will forget to update it. Only crashing could warn you to do something.

@jesseduffield
Copy link
Owner

Could we add a specific step in the CI to fail if that go get step fails?

@antham
Copy link
Contributor Author

antham commented Sep 7, 2018

yes we can

@antham
Copy link
Contributor Author

antham commented Sep 7, 2018

@jesseduffield if you can have a look now

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

LGTM :)

@jesseduffield jesseduffield merged commit 5af03b6 into jesseduffield:master Sep 9, 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.

4 participants