Formats each .go file in the repo #83

sloum · 2019-11-10T18:48:46Z

sloum commented

2019-11-10 18:48:46 +00:00

It turned out my version of spacevim had not updated the necessary binaries for running go fmt nor a number of other good language specific features. I got that all sorted and while in there formatted each file. I'll leave this open for a day so anyone that wants to can look at it, but I dont imagine there are any concerns here. I'll merge it in after that time.

sloum added this to the 2.0.0 milestone 2019-11-10 18:48:46 +00:00

sloum added the

non-functional

label 2019-11-10 18:48:46 +00:00

sloum commented

2019-11-10 19:37:01 +00:00

I also ran GoMetaLinter and handled a number (though nowhere near all) of the issues it brought up.

Some of them I take issue with and may not update, but I have updated a bunch and will do more later today.

I also ran `GoMetaLinter` and handled a number (though nowhere near all) of the issues it brought up. Some of them I take issue with and may not update, but I have updated a bunch and will do more later today.

asdf commented

2019-11-10 21:44:53 +00:00

Excellent! Glad you are able to run gofmt automatically, that will help a lot. It would be a good addition to the dev guide too.

I did have a look at making some changes with GoMetaFilter. This takes care of a good chunk of them.

A couple of outstanding items I wanted to ask about:

client.go|271 col 21| error strings should not be capitalized or end with punctuation or a newline (golint)

This rule seems very picky, but makes sense in its definition. I'm not sure how this message appears though.

client.go|480 col 18| method getCurrentPageUrl should be getCurrentPageURL (golint)

There's quite a few "Url" to "URL" messages, explained here. I'm not too concerned about this one, but understand the point. If it is changed, a reliable find and replace is required globally.

cui.go - funcs drawShape and moveThenDrawShape are unused.

Can these be removed? I tested and it seems like it can be.

cui_test.go is empty

Should we remove this file too?

Excellent! Glad you are able to run `gofmt` automatically, that will help a lot. It would be a good addition to the dev guide too. I did have a look at making some changes with GoMetaFilter. This takes care of a good chunk of them. A couple of outstanding items I wanted to ask about: - `client.go|271 col 21| error strings should not be capitalized or end with punctuation or a newline (golint)` This rule seems very picky, but makes sense in its [definition](https://github.com/golang/go/wiki/CodeReviewComments#error-strings). I'm not sure how this message appears though. - `client.go|480 col 18| method getCurrentPageUrl should be getCurrentPageURL (golint)` There's quite a few "Url" to "URL" messages, explained [here](https://github.com/golang/go/wiki/CodeReviewComments#initialisms). I'm not too concerned about this one, but understand the point. If it is changed, a reliable find and replace is required globally. - cui.go - funcs drawShape and moveThenDrawShape are unused. Can these be removed? I tested and it seems like it can be. - cui_test.go is empty Should we remove this file too?

sloum commented

2019-11-10 22:13:40 +00:00

Yeah. I saw those too and skipped over them. The error one... well, I'm passing the error message to the client, so it makes sense to follow normal punctuation rules for english, not for developers. It is possible that we could catch an error and then provide messaging where the error is caught, but that seems needless. I'm for ignoring that one.
I wasn't sure how to feel about the Url vs URL ones. I'm no necessarily opposed to updating that, my in-law showed up and I had to stop working on things for a bit... what do you think? Do a find and replace and update it or decide it is fine?
Yes. Those can likely be removed, in deference to YAGNI. They exist from the 1.0 release. Originally cui was meant to be a standalone library, but just kind of turned into the rendering engine for v1. Now it mostly just houses things that interact with terminal settings + a bit extra. I'm fine with their removal at this point.
I'll remove the test and we can always add back in if we end up needing it.

EDIT: I went through and finished updating the files to pass the linter. I left the Url stuff in. I removed the exclamation point from that error and that satisfied the linter. I am also ok with that middle ground. Pretty much everything that is left I am ok with being there, though they will always be annoying things now if we leave them there :-/

- Yeah. I saw those too and skipped over them. The error one... well, I'm passing the error message to the client, so it makes sense to follow normal punctuation rules for english, not for developers. It is possible that we could catch an error and then provide messaging where the error is caught, but that seems needless. I'm for ignoring that one. - I wasn't sure how to feel about the Url vs URL ones. I'm no necessarily opposed to updating that, my in-law showed up and I had to stop working on things for a bit... what do you think? Do a find and replace and update it or decide it is fine? - Yes. Those can likely be removed, in deference to YAGNI. They exist from the 1.0 release. Originally cui was meant to be a standalone library, but just kind of turned into the rendering engine for v1. Now it mostly just houses things that interact with terminal settings + a bit extra. I'm fine with their removal at this point. - I'll remove the test and we can always add back in if we end up needing it. **EDIT**: I went through and finished updating the files to pass the linter. I left the `Url` stuff in. I removed the exclamation point from that error and that satisfied the linter. I am also ok with that middle ground. Pretty much everything that is left I am ok with being there, though they will always be annoying things now if we leave them there :-/

asdf approved these changes 2019-11-12 01:44:53 +00:00

asdf left a comment

This looks good - can't see any issues, nothing seems different when using this branch.

sloum closed this pull request

2019-11-12 02:42:30 +00:00

sloum deleted branch run-format

2019-11-12 02:42:37 +00:00

Sign in to join this conversation.

No reviewers