cycloidio / raws

[UNMAINTAINED] AWS Reader
MIT License
15 stars 0 forks source link

Beta #1

Closed xlr-8 closed 7 years ago

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 0cdf33080c1cb79fe634f1fb4470e940116d2379 on beta into on master.

xescugc commented 7 years ago

Overall I see no errors, just some punctuations (it's a big PR hehe)

For the documentation I recomend you to read this post which explains the go way.

Notice this comment is a complete sentence that begins with the name of the element it describes. This important convention allows us to generate documentation in a variety of formats, from plain text to HTML to UNIX man pages, and makes it read better when tools truncate it for brevity, such as when they extract the first line or sentence.

For the code everithing is clear (I read it in an overview as its big) but I think that it's well formatted, ther are some funcions in which you could use a go syntax suggar on declare repeated types on the arguments, example you could do:

   func NewConnector(accessKey, secretKey string, regions []string, config *aws.Config) (*Connector, error) { ... }

instead of

  func NewConnector(accessKey string, secretKey string, regions []string, config *aws.Config) (*Connector, error) {

Other things are opiniated, the use of var instead of := everyone has an opinion on that :)

xlr-8 commented 7 years ago

Thank you for the comments, I'll take a look at the doc link.

Regarding the notation, for me it really depends of the situation. When the variables are just declared, and used later on, then it makes sense to me to do so, I find it also more explicit when using those in an example. However for directly used/initialized variable within the code, then I don't see the problem with that, as it's self-explicit.

And on the arguments, I personally prefer to use the shortened version, only when all arguments have the same type, I find it confusing otherwise. But as you said, everyone has his opinion :)

ifraixedes commented 7 years ago

I also prefer to use the long syntax to declare variables var ... and only to use the short syntax for loops (for i := ...) and conditionals (if err :=) because

  1. Go doesn't compile if you use var on loops on conditionals e.g. for var i = ...
  2. Even though it would compile, I'm not sure if having the var word after for and if could be quite unreadable.

Obviously 1 enforce to use the short syntax on such cases if you only want to declare the variable inside of the loop and conditional scope

Regarding the Go syntax sugar for the types of the function arguments, I wold have prefer that it won't exist, so I prefer to use the long syntax, rather than the short, although it's something that, at the beginning I used the short syntax, later I've been doubting between both and now I prefer the long one.

The case that @xlr-8 mentions could be a good case to consider to use it, however I'm still not sold on using both rather than one.

As you commented this is a personal opinion, so I don't think that we should enforce one, they are minor things to remember and because Go has a short set of instructions and statements and syntax sugar tricks, every Go developer know them, so the syntax looks familiar. If you would like to enforce one, I'm not agains but only if we can detect them through a linter.