Error Handling and unused code removal #3

jboverf · 2019-05-01T00:24:54Z

jboverf commented

2019-05-01 00:24:54 +00:00

Added some error handling and removed unused code

sloum reviewed 2019-05-01 04:10:46 +00:00

sloum left a comment

This looks great! I'm trying to figure out a good way to build your updates. I restructured my src folder to follow the go/src/{githost}/{user}/{repo} format; the way the imports you set up wants. But I then realized you were working off of a clone and doing a PR into my repo. So I cloned your repo onto my local system and tried to build, but the imports point to my repo and I get errors because the updated code isnt there.

Outside of work (where we are all on the same repos/workgroups) I have not done much collaboration (beyond the odd PR I've done into other peoples projects). Can you advise what the best way to work on this would be? I dont know if tildegit supports multiple people being added to a repo directly. If so, that could be a possibility. Otherwise maybe me adding a develop branch and doing PRs into that so as to not merge into master directly before building and testing code?

I'd appreciate any thoughts you have on the subject as you seem more experienced than me at it.

This looks great! I'm trying to figure out a good way to build your updates. I restructured my src folder to follow the go/src/{githost}/{user}/{repo} format; the way the imports you set up wants. But I then realized you were working off of a clone and doing a PR into my repo. So I cloned your repo onto my local system and tried to build, but the imports point to my repo and I get errors because the updated code isnt there. Outside of work (where we are all on the same repos/workgroups) I have not done much collaboration (beyond the odd PR I've done into other peoples projects). Can you advise what the best way to work on this would be? I dont know if tildegit supports multiple people being added to a repo directly. If so, that could be a possibility. Otherwise maybe me adding a develop branch and doing PRs into that so as to not merge into master directly before building and testing code? I'd appreciate any thoughts you have on the subject as you seem more experienced than me at it.

cui/cui.go Outdated

						
				@ -138,1 +153,3 @@

					fmt.Print("33[?25l")

					err := cmd.Run()

					if err != nil {

						return err

sloum commented

2019-05-01 03:42:16 +00:00

It is possible that this should be panic rather than return an error. If it fails trying to enter cbreak/noecho, control of the program will at best be pretty funky and at worst be so confusing for a user that they have trouble exiting.

It is possible that this should be `panic` rather than return an error. If it fails trying to enter cbreak/noecho, control of the program will at best be pretty funky and at worst be so confusing for a user that they have trouble exiting.

cui/cui.go

						
				@ -143,3 +164,3 @@

					cmd.Stdin = os.Stdin

					cmd.Stdout = os.Stdout

					cmd.Run()

					return cmd.Run()

sloum commented

2019-05-01 03:44:05 +00:00

In the above code for func SetCharMode() error handling was added to the cmd.Run() call. Adding here would make sense from a consistency standpoint, but curious if it was left out intentionally?

In the above code for `func SetCharMode()` error handling was added to the `cmd.Run()` call. Adding here would make sense from a consistency standpoint, but curious if it was left out intentionally?

jboverf commented

2019-05-01 11:40:26 +00:00

Well this block didn't have that Print statement afterwards, so I'm able to just return the error directly. I couldn't do that in SetCharMode because fmt.Print returns two values. Now that I'm looking at it it probably makes sense to just panic, since you're right if the line modes can't be set we can't really go forward. I'll make those changes (which will also simplify error handling for anything that calls these functions)

sloum commented

2019-05-01 16:28:18 +00:00

Awesome! Thanks!

main.go

						
				@ -128,4 +128,1 @@

					if screen.Activewindow == 0 {

					} else {

					}

sloum commented

2019-05-01 03:48:13 +00:00

Hahaha I do not know how I did not notice this sitting in there.

main.go

						
				@ -399,0 +431,4 @@

					if err != nil {

						// if we can't initialize the window,

						// we can't do anything!

						panic(err)

sloum commented

2019-05-01 03:51:27 +00:00

:thumbsup

jboverf commented

2019-05-01 11:37:59 +00:00

Your right testing branches is a little weird, but modules help here! Check my fork out somewhere else outside of GOPATH (provided you're building this with at least Go 1.11). Then switch to my error-handling branch, and run go build as normal.

I saw you gave me access to the repo - that will make this easier and sidestep the need to make a new checkout somewhere else on your system. I will continue to make PRs for any changes (I'll never to commit to master without your sign off).

Your right testing branches is a little weird, but modules help here! Check my fork out somewhere else outside of GOPATH (provided you're building this with at least Go 1.11). Then switch to my error-handling branch, and run `go build` as normal. I saw you gave me access to the repo - that will make this easier and sidestep the need to make a new checkout somewhere else on your system. I will continue to make PRs for any changes (I'll never to commit to master without your sign off).

sloum commented

2019-05-01 14:35:41 +00:00

Ah. That must be part of the problem. I seem to be on Go 1.10. I dont seem to have installed it via apt for some reason and so it hasnt been getting upgraded. So now I have to figure out how to remove it, and get the apt version.

Once I do, I'll try what you suggested!

Yes! You have write access. :) In general I will also issue PRs for Master (if for no other reason than to have a commented record of the update). I hadnt done that at first, but have been wanting to shift that way. If you are not opposed, I will request review from you as well (it doesnt have to be an intense review, I'm not wanting to pull a ton of your time... Im sure you are busy).

Thanks again for all the help!

Ah. That must be part of the problem. I seem to be on Go 1.10. I dont seem to have installed it via apt for some reason and so it hasnt been getting upgraded. So now I have to figure out how to remove it, and get the apt version. Once I do, I'll try what you suggested! Yes! You have write access. :) In general I will also issue PRs for Master (if for no other reason than to have a commented record of the update). I hadnt done that at first, but have been wanting to shift that way. If you are not opposed, I will request review from you as well (it doesnt have to be an intense review, I'm not wanting to pull a ton of your time... Im sure you are busy). Thanks again for all the help!

jboverf commented

2019-05-01 15:37:26 +00:00

Definitely happy to review anything you send my way! I don't have Go installed via the package manager either - I tend to just download the Linux amd64 binary tarball from the Go site and copy it to ~/go after untaring it. Then ~/go/bin is on my PATH. My GOPATH is ~/gospace but anything works really.

sloum commented

2019-05-01 15:45:26 +00:00

Ok. Got to work and spent some time upgrading my setup. I am still extremely confused on where things get stored and built... however, like you said: building your repo version did work. So that is good news :)

Ok. Got to work and spent some time upgrading my setup. I am still extremely confused on where things get stored and built... however, like you said: building your repo version _did_ work. So that is good news :)

jboverf commented

2019-05-01 22:31:54 +00:00

Alright I pushed updates to the branch - those cbreak functions should panic now on any error.

There's been quite a bit of upheaval (in a good way imo) in the Go community recently with the introduction of modules. I think it hits people especially hard that don't use it every day and learned a while ago how to use GOPATH effectively. The easiest way I can describe it is if you're opted in to Go Modules (i.e. you have a go.mod file and are using Go 1.11+), you can start putting Go code in directories like ~/src or anywhere you kept code for other languages. No need for a separate "special" place anymore.

I believe it caches intermediate build/compile artifacts in $XDG_CACHE_HOME/go-build, which for me is ~/.cache/go-build

Alright I pushed updates to the branch - those cbreak functions should panic now on any error. There's been quite a bit of upheaval (in a good way imo) in the Go community recently with the introduction of modules. I think it hits people especially hard that don't use it every day and learned a while ago how to use GOPATH effectively. The easiest way I can describe it is if you're opted in to Go Modules (i.e. you have a go.mod file and are using Go 1.11+), you can start putting Go code in directories like ~/src or anywhere you kept code for other languages. No need for a separate "special" place anymore. I believe it caches intermediate build/compile artifacts in $XDG_CACHE_HOME/go-build, which for me is ~/.cache/go-build

sloum commented

2019-05-01 23:19:30 +00:00

Awesome. Thanks again for the details! Merging in now. :)

sloum closed this pull request

2019-05-01 23:19:37 +00:00

Sign in to join this conversation.

No reviewers