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

envy.Mods() - Modify behavior to better match the current module auto… #25

Merged
merged 1 commit into from
Apr 16, 2019
Merged

Conversation

chmorgan
Copy link
Contributor

…detection in place in go

Combined with packr2 this fixes an issue I was seeing with incorrect imports in main-packr.go.

This hung me up for a few hours today and may be the reason some others have ran into the same thing. The go devs have changed the module support to autodetect I think in 1.12 and imo we may want to keep pace with that as its what people are expecting to see.

We could also improve this to only perform this auto detection if using 1.12 (or whatever version introduced the auto module support) if it is important to maintain behavior.

@markbates
Copy link
Member

Note: GO111MODULE=auto is not supported - that's the whole point of this PR, you are enabling that mode. That's how auto works, which was first introduced in 1.11 and will be switched to on in 1.13. This PR enables support outside of the GOPATH if GO111MODULE != on, which is almost akin to auto, except that it doesn't enable a person to actually turn modules off when outside of their GOPATH.

This PR says, regardless of what you've set GO111MODULE to as long as you are outside of your GOPATH they're going to be used, which isn't correct.

@chmorgan
Copy link
Contributor Author

@markbates ahh right, let me fix that.

@chmorgan
Copy link
Contributor Author

@markbates yeah I'm not sure how to say that we support go modules auto detection if GO111MODULE is unset but not if GO111MODULE=auto. I just removed that text but I can replace if it you have suggestions.

I pushed to kick off the automated tests. I'll look into what is going on there.

@chmorgan
Copy link
Contributor Author

@markbates ready for re-review, tests are passing.

@markbates
Copy link
Member

Can you please write some tests? Also, what if “auto” is set, which is the Go default, and I’m outside the GOPATH? It should work, but I don’t think it will.

Thanks.

@chmorgan
Copy link
Contributor Author

@markbates I think I addressed the issue with setting GO111MODULES to 'auto' and added a few obvious tests. Thoughts?

@markbates
Copy link
Member

markbates commented Apr 2, 2019

Sorry for the delay. The logic feels a little strange to me.

Perhaps something like this:

if not go path {
return mods != "off"
} else {
return mods == on
}

@chmorgan
Copy link
Contributor Author

chmorgan commented Apr 2, 2019

@markbates yeah that is a more simple way to put it, let me fix up and repush.

…detection in place in go

Add tests for GO111MODULE being set to auto or being left blank.
@chmorgan
Copy link
Contributor Author

Hi @markbates, any other comments or fixes you’d like me to make to this pr?

@markbates
Copy link
Member

markbates commented Apr 12, 2019 via email

@markbates markbates merged commit 56bafbd into gobuffalo:master Apr 16, 2019
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.

None yet

2 participants