Minor clean / tidying up the code #113

creack · 2020-09-04T15:38:58Z

Scope errors when possible.
Fix nil pointers in unmarshal.
Align struct tags for readability.
Explicitly discard errors with a comment on why.
Import package order.
Fmt verb for types.
More comments.

cmoog

Thanks 😄

coder-sdk/secrets.go

internal/cmd/auth.go

cmoog · 2020-09-04T16:48:03Z

internal/cmd/shell.go

-			if err != nil {
-				flog.Fatal("\nFailed to ping websocket: %v, exiting...", err)
+			if err := conn.Ping(ctx); err != nil {
+				// TODO: Don't fatal, it would skip the TTY state restore and result in a broken terminal.


Ah! Nice catch.

coadler

Just some random typos I found. Great improvements 😃

coadler · 2020-09-04T17:19:29Z

ci/integration/setup_test.go

@@ -32,7 +32,7 @@ func init() {

 // build the coder-cli binary and move to the integration testing bin directory
 func build(path string) error {
-	tar := fmt.Sprintf("coder-cli-linux-amd64.tar.gz")


Yeah, that one is particularly embarrassing... leftover from when it was MacOS and Linux. 😭

coder-sdk/devurl.go

internal/x/xterminal/terminal_windows.go

tychoish · 2020-09-04T20:09:29Z

cmd/coder/main.go


 func main() {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()

+	// If requested, spin up the pprof webserver.
 	if os.Getenv("PPROF") != "" {
 		go func() {


I wish we put panic handlers in all go routines.

Hmm, panics are developer errors, not really expected in production. As a safety net on a service to log/alert about a bug makes sense, and in general, for anything expected to be long-running, I do agree, we should have panic handler, but here, we are in a CLI and specifically in a debug handler, so it is probably not necessary.

tychoish · 2020-09-04T20:11:58Z

cmd/coder/main.go

 	if os.Getenv("PPROF") != "" {
 		go func() {
 			log.Println(http.ListenAndServe("localhost:6060", nil))
 		}()
 	}

+	// TODO: Document why this is needed. Only does something on windows.


I'd be inclined to avoid adding TODOs to the code base? I think it's better to either do the thing if its small or open issues?

Will remove the todos in favor of issues.

tychoish · 2020-09-04T20:12:37Z

cmd/coder/main.go

-	err = app.ExecuteContext(ctx)
-	if err != nil {
+	if err := app.ExecuteContext(ctx); err != nil {
+		// NOTE: The cmd lib will handle the logs, no need to do it here as well, simply exit with error code.


what about this error? is it interesting?

I'll reword the comment to be clearer. That error is already checked and log in the cmd lib. It is indeed an interesting one :)

coder-sdk/client.go

coder-sdk/env.go

tychoish · 2020-09-04T20:23:06Z

internal/cmd/shell.go

-				flog.Fatal("\nFailed to ping websocket: %v, exiting...", err)
+			if err := conn.Ping(ctx); err != nil {
+				// TODO: Don't fatal, it would skip the TTY state restore and result in a broken terminal.
+				// NOTE: Prefix with \r\n to attempt to have clearer output as we might still be in raw mode.


can the prefix be a constant that we can reuse for these kinds of situations?

This is a one-off that should be avoided, as we should control how/what we output. Promoting this to a const would imply it is supported and might encourage it to be used and for just two chars, especially CRLF, I am not sure it would add value.

I think putting "LineFeed" as constant gives it a name which makes the comment less required?

I feel like adding \r\n to the ends of something is never just a one-off, but I'm far less familiar with this code base.

tychoish · 2020-09-04T20:23:35Z

internal/sync/sync.go

-	if err != nil {
+	if err = process.Wait(); err != nil {
+		if code, ok := err.(wsep.ExitError); ok {
+			return fmt.Errorf("%s exit status: %d", prog, code)


we should be consistent, I think about which Errorf questions.

I was wondering about it, as it was already there, I opted not to change it as there might have been a reason?

I'm not sure, I can't imagine a way that it would matter, and it feels like this is the right kind of PR to make that change. I don't feel particularly strongly at all.

tychoish · 2020-09-04T20:24:03Z

internal/sync/sync.go

 		return err
 	}
 	return nil
 }

 func (s Sync) handleDelete(localPath string) error {
-	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)


I wouldn't multiply by 1?

for consistency and readability, I think it is best to always specify the full time, it shows it is intended and not an oversight. As it is a const, this is handled at compile time and have no impact and the runtime, it is purely aesthetic.

With this in mind, if you think it is best to remove it, we sure can.

I'm still for removing, in a "less is more" sort of approach.

tychoish · 2020-09-04T20:25:32Z

internal/sync/sync.go

@@ -215,7 +211,7 @@ var ErrRestartSync = errors.New("the sync exited because it was overloaded, rest

 // workEventGroup converges a group of events to prevent duplicate work.
 func (s Sync) workEventGroup(evs []timedEvent) {
-	cache := make(eventCache)
+	cache := make(eventCache, len(evs))


what Add do? this seems suspicious?

We are creating a map where we know exactly how big it is going to be in advance, so we can pre-allocate the memory for it.

Interesting, I guess not seeing what type eventCache is makes this harder. I'm all down for preallocation, but we're far enough from the context that it was hard to read/guess.

Frankly, it'd make more sense if there was a constructor that did all of this, rather than just having it at the beginning of a worker pool.

tychoish · 2020-09-04T20:29:02Z

internal/x/xterminal/terminal_windows.go

-	events := make(chan ResizeEvent)
-	ticker := time.NewTicker(time.Millisecond * 100)
+func ResizeEvents(ctx context.Context, termFD uintptr) chan ResizeEvent {
+	events := make(chan ResizeEvent, 1) // Use a buffered chan to avoid blocking.


isn't blocking a symptom of a bug? if it works without buffering, I would leave it buffered?

I'll update the comment to add more context about the reasoning.

We want to send an initial event to signal the current size of the window. I was planning to do this in a different PR as this one ended up being mostly about styling/cleaning.

At a higher level, the idea is that when dealing with the channel, the runtime notifies us via a non-blocking chan, so if we are consuming it on an unbuffered chan, we lose the event.
In this specific case, we are dealing with windows and it is not actually a signal, but it maps directly with how it works on unix.

tychoish

I don't think anything is blocking merging this, but we can talk a little bit more about any of the details. I think most of these changes are an improvement.

tychoish · 2020-09-08T14:11:58Z

coder-sdk/request.go

 	if err != nil {
 		return nil, xerrors.Errorf("create request: %w", err)
 	}
+	// TODO: Consider setting content-type / encoding / accept headers.
+
+	// Execute the request.


I'm still for removing.

Comments are just one more thing that can get crufty, and while I agree that the interface is intuitive, I would hate that we should sprinkle descriptions of other package's APIs throughout calling code.

tychoish · 2020-09-08T14:13:15Z

internal/activity/pusher.go

 	"go.coder.com/flog"
 )

-const pushInterval = time.Minute
+const pushInterval = 1 * time.Minute


I always assume that people typo'd 10*time.Minute here, and think that time constants are clear enough.

tychoish · 2020-09-08T14:13:37Z

internal/activity/pusher.go

@@ -19,27 +20,28 @@ type Pusher struct {
 	source string

 	client *coder.Client
-	rate   *rate.Limiter
+	rate   *rate.Limiter // Use a rate limiter to control the sampling rate.


I would omit this comment too.

tychoish · 2020-09-08T14:17:02Z

internal/cmd/cmd.go

@@ -92,13 +91,13 @@ $ coder completion fish > ~/.config/fish/completions/coder.fish
 	Run: func(cmd *cobra.Command, args []string) {
 		switch args[0] {
 		case "bash":
-			cmd.Root().GenBashCompletion(os.Stdout)
+			_ = cmd.Root().GenBashCompletion(os.Stdout) // Best effort.


I think putting a higher level comment that says "we ignore errors when generating completion because they're not critical and the tool degrades gracefully" is very clear, whereas "best effort" doesn't really capture that decision or background as clearly (particularly when redundant.)

I think the ignored error itself does a pretty good job of communicating intent, though I agree that some kind of comment is useful.

I'm not strongly committed to this view.

tychoish · 2020-09-08T14:19:18Z

internal/cmd/shell.go


-	tty := terminal.IsTerminal(int(termfd))
+	// Check if the client is running an interactive session.


The best way to avoid having out of date comments is to avoid writing code that needs comments :)

Perhaps the tty variable would be more clear if it was named isInteractive or something similar. It could also be combined onto the next line.

I think given how internal/low level (I suppose), I think it's fine

tychoish · 2020-09-08T14:21:21Z

internal/cmd/shell.go

-				flog.Fatal("\nFailed to ping websocket: %v, exiting...", err)
+			if err := conn.Ping(ctx); err != nil {
+				// TODO: Don't fatal, it would skip the TTY state restore and result in a broken terminal.
+				// NOTE: Prefix with \r\n to attempt to have clearer output as we might still be in raw mode.


I think putting "LineFeed" as constant gives it a name which makes the comment less required?

I feel like adding \r\n to the ends of something is never just a one-off, but I'm far less familiar with this code base.

tychoish · 2020-09-08T14:22:37Z

internal/sync/sync.go

-	if err != nil {
+	if err = process.Wait(); err != nil {
+		if code, ok := err.(wsep.ExitError); ok {
+			return fmt.Errorf("%s exit status: %d", prog, code)


I'm not sure, I can't imagine a way that it would matter, and it feels like this is the right kind of PR to make that change. I don't feel particularly strongly at all.

tychoish · 2020-09-08T14:23:25Z

internal/sync/sync.go

 		return err
 	}
 	return nil
 }

 func (s Sync) handleDelete(localPath string) error {
-	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)


I'm still for removing, in a "less is more" sort of approach.

tychoish · 2020-09-08T14:25:26Z

internal/sync/sync.go

@@ -215,7 +211,7 @@ var ErrRestartSync = errors.New("the sync exited because it was overloaded, rest

 // workEventGroup converges a group of events to prevent duplicate work.
 func (s Sync) workEventGroup(evs []timedEvent) {
-	cache := make(eventCache)
+	cache := make(eventCache, len(evs))


Interesting, I guess not seeing what type eventCache is makes this harder. I'm all down for preallocation, but we're far enough from the context that it was hard to read/guess.

Frankly, it'd make more sense if there was a constructor that did all of this, rather than just having it at the beginning of a worker pool.

- Scope errors when possible. - Fix nil pointers in unmarshal. - Align struct tags for readability. - Explicitly discard errors with a comment on why. - Import package order. - Fmt verb for types. - More comments. Signed-off-by: Guillaume J. Charmes <guillaume@coder.com>

…cleanup-tidy-up

creack requested review from cmoog and f0ssel September 4, 2020 15:38

creack force-pushed the minor-cleanup-tidy-up branch 2 times, most recently from b1b256c to e9acc2d Compare September 4, 2020 16:29

cmoog approved these changes Sep 4, 2020

View reviewed changes

creack requested a review from coadler September 4, 2020 17:06

coadler approved these changes Sep 4, 2020

View reviewed changes

tychoish suggested changes Sep 4, 2020

View reviewed changes

This was referenced Sep 5, 2020

Avoid fatal when checking for auth #114

Closed

standardize string concatenation vs sprintf #115

Closed

creack force-pushed the minor-cleanup-tidy-up branch 4 times, most recently from 24742ea to a89511e Compare September 5, 2020 22:08

tychoish approved these changes Sep 8, 2020

View reviewed changes

coder-sdk: remove spurious JSON tag

ad78142

creack force-pushed the minor-cleanup-tidy-up branch from b4a7ef8 to fe41093 Compare September 15, 2020 17:04

Cleanup:

6fb7c7b

- Scope errors when possible. - Fix nil pointers in unmarshal. - Align struct tags for readability. - Explicitly discard errors with a comment on why. - Import package order. - Fmt verb for types. - More comments. Signed-off-by: Guillaume J. Charmes <guillaume@coder.com>

creack force-pushed the minor-cleanup-tidy-up branch from fe41093 to 6fb7c7b Compare September 15, 2020 17:15

Merge branch 'alrs-json-tag' of github.com:alrs/coder-cli into minor-…

2b4ec5b

…cleanup-tidy-up

creack merged commit c796bca into master Sep 15, 2020

creack deleted the minor-cleanup-tidy-up branch September 15, 2020 17:20


		tty := terminal.IsTerminal(int(termfd))
		// Check if the client is running an interactive session.

Minor clean / tidying up the code #113

Minor clean / tidying up the code #113

Conversation

creack commented Sep 4, 2020

cmoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coadler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tychoish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment