Skip to content
This repository has been archived by the owner on Jan 17, 2021. It is now read-only.

Create base #1

Merged
merged 4 commits into from
Apr 19, 2019
Merged

Create base #1

merged 4 commits into from
Apr 19, 2019

Conversation

kylecarbs
Copy link
Member

No description provided.

@kylecarbs kylecarbs requested a review from nhooyr April 19, 2019 00:54
main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

🔥

main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated
}

flog.Info("starting code-server...")
localPort := scanAvailablePort()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just use :0 to make ssh automatically use an available port. This is better than this as with this, a user on macOS will get a prompt asking if they want to let sshcode listen on the network, and then get another prompt for the ssh binary which is confusing.

main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@nhooyr
Copy link
Contributor

nhooyr commented Apr 19, 2019

Not bad at all for first time Go btw, most of my issues are just nitpicks.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
`, os.Args[0])
}

flag.Parse()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but in the future, you don't need the flag library, you have no flags. You can directly use os.Args.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, flag automatically gives you a -h flag which is nice. So this is the way to go.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

make sure you run gofmt -s -w . on the code.

Otherwise looks solid to me.

Congrats on passing your first @nhooyr software review 🎉 🔥

`, os.Args[0])
}

flag.Parse()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, flag automatically gives you a -h flag which is nice. So this is the way to go.

var openCmd *exec.Cmd
if commandExists("google-chrome") {
openCmd = exec.Command("google-chrome", "--app="+url, "--disable-extensions", "--disable-plugins")
} else if commandExists("firefox") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it back. If neither of these exist, this program will panic.

Copy link
Contributor

Choose a reason for hiding this comment

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

var openCmd *exec.Cmd
if commandExists("google-chrome") {
openCmd = exec.Command("google-chrome", "--app="+url, "--disable-extensions", "--disable-plugins")
} else if commandExists("firefox") {
Copy link
Contributor

Choose a reason for hiding this comment

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

main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants