Skipper: Improve linting

Created on 17 May 2016  路  8Comments  路  Source: zalando/skipper

As of current version (08b8adf04c1d7ca80ae76df6adc757c80627270f), skipper needs some linting love: it currently has 278 "problems", mostly style mistakes like proxy/proxy.go:228:6: func cloneUrl should be cloneURL.
I would go through all the source code and make it compliant. Please @lmineiro @aryszka have a look at the current output and share what you think.

enhancement help wanted

Most helpful comment

Hi All! I ran some grep commands on the golint output and identified the following kinds of warnings:

  1. Public function/method/type should have comment or be unexported
  2. Malformed comment on method/function/type
  3. Receiver name should not be an underscore
  4. Inconsistent reciever names
  5. Should omit 2nd value from range
  6. Exported func New returns unexported type
  7. Error vars should be prefixes with err
  8. If block ends with a return statement, so drop this else and outdent its block
  9. Replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)
  10. Violated naming conventions of private symbol (Url vs URL)
  11. Violated naming conventions of public symbol (Url vs URL)

As far as I can tell warnings 1-9 can be fixes without introducing API breaks or inconsistencies. If there are no objections I would like to tackle warnings 1-9 and use this issue as an opportunity to make a small tour through the Skipper source code.

All 8 comments

Most of it won't hurt. I'd reconsider any of those changes that can break backwards compatibility. I'd rather have the linter bitching about them

Agreed, I also briefly discussed in private with @aryszka. The only thing I think that we really need is to make sure that struct initialization is not positional anymore.

the only thing that I found that would break backwards compatibility is renaming some public symbols (e.g. ._ID, ._URL, Err.*). These can break programs that depend on skipper as a library. The rest can be fixed anytime. For the public symbols, I recommend to do it in v2 (which is my summer mission :)). Many Thanks @Raffo

@aryszka @Raffo Should we apply a "Help Wanted" label restating the precise help we'd want?

Hi All! I ran some grep commands on the golint output and identified the following kinds of warnings:

  1. Public function/method/type should have comment or be unexported
  2. Malformed comment on method/function/type
  3. Receiver name should not be an underscore
  4. Inconsistent reciever names
  5. Should omit 2nd value from range
  6. Exported func New returns unexported type
  7. Error vars should be prefixes with err
  8. If block ends with a return statement, so drop this else and outdent its block
  9. Replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)
  10. Violated naming conventions of private symbol (Url vs URL)
  11. Violated naming conventions of public symbol (Url vs URL)

As far as I can tell warnings 1-9 can be fixes without introducing API breaks or inconsistencies. If there are no objections I would like to tackle warnings 1-9 and use this issue as an opportunity to make a small tour through the Skipper source code.

That would be greatly appreciated @stoewer. Eager for the PR

@szuecs @aryszka I'm not sure if you still need this issue, but I unassigned myself... It's unlikely I'm gonna work on it :sweat_smile:

I will close it.
We have more tooling to have forced linting, but some parts will only change if we have a breaking change.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

patrickdk77 picture patrickdk77  路  7Comments

krism74 picture krism74  路  9Comments

herojan picture herojan  路  7Comments

azarakovskiy picture azarakovskiy  路  6Comments

slok picture slok  路  4Comments