whyrusleeping / gx

A package management tool
MIT License
1.88k stars 111 forks source link

Use pointers for CLI flags #242

Closed lion7 closed 4 years ago

lion7 commented 4 years ago

urfave/cli v2 has an incompatible change, for details see https://github.com/urfave/cli/issues/459

lion7 commented 4 years ago

Hmm, after opening this PR I discovered the existence of Go Modules and that you explicitly have to enable them (sorry, n00b Go user here).

Anyway, just installing the latest Go binary and running the go get github.com/whyrusleeping/gx command doesn't work. I first had to run set GO111MODULE=on (on Windows) or export GO111MODULE=on (on Linux). Maybe this should be added to the README instead of this PR? Or maybe complementary?

Stebalien commented 4 years ago

It should work with GO111MODULE=auto, which has been the default since go 1.13. Is that not the case?

Stebalien commented 4 years ago

Anyways, I'd actually be happy to upgrade to cli v2 (but the go.mod file needs to be updated as well).

lion7 commented 4 years ago

I double checked the Go documentation and it does indeed state that auto should be the default for GO111MODULE. But apparently that doesn't work for me. Steps I took to install gx:

lion7 commented 4 years ago

I tried to upgrade the CLI to v2, but ran into more and more issues. Let me try some more tomorrow.

Oh, and I also made the commit history a mess, so I'll probably squash them and do a force push somewhere. Or just a new PR if that doesn't work.

Stebalien commented 4 years ago

Can you double check go version?

lion7 commented 4 years ago

Sure:

C:\Users\gerar>go version
go version go1.14.1 windows/amd64
lion7 commented 4 years ago

Ok, I think I now properly upgraded the dependency and fixed all compile errors. Force pushing also worked so the commit log should be clean now.

@Stebalien Can you please do a thorough review? This is my first time ever doing something in Go 😉

lion7 commented 4 years ago

Oh lol, you already reviewed while I was typing this 😆 Anyway, Travis CI still fails because the way short flags work have changed. Gonna fix that first.

Stebalien commented 4 years ago

Lol, we keep speaking past each other.

C:\Users\gerar>go version go version go1.14.1 windows/amd64

:man_shrugging:

lion7 commented 4 years ago

Travis says OK :) BTW, the tests don't work locally on Windows. I think that's not really an issue, but just wanted to mention it.

Stebalien commented 4 years ago

BTW, the tests don't work locally on Windows. I think that's not really an issue, but just wanted to mention it.

That's not all that surprising.

Stebalien commented 4 years ago

If you want to try to fix the tests on windows, that would be awesome! But we've never bothered putting any work into them.

lion7 commented 4 years ago

Haha, sure. If I get bored of sitting at home all day I might take a look at it 😉