Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix linting issues reported by make precommit #838

Closed
41 of 46 tasks
dliappis opened this issue Dec 12, 2019 · 3 comments
Closed
41 of 46 tasks

Fix linting issues reported by make precommit #838

dliappis opened this issue Dec 12, 2019 · 3 comments
Labels
enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc.
Milestone

Comments

@dliappis
Copy link
Contributor

dliappis commented Dec 12, 2019

Now that we got infrastructure to enforce coding conventions in #794 we need to start coming up with PRs to fix issues. This issue tracks the effort and should be only closed when make precommit succeeds with no errors reported.

@dliappis dliappis added enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc. labels Dec 12, 2019
@dliappis dliappis added this to the 1.4.0 milestone Dec 12, 2019
@dliappis
Copy link
Contributor Author

dliappis commented Dec 12, 2019

@drawlerr some top level guidelines to make the corresponding PRs more manageable to review and focus on important topics:

  1. This is a frequency chart of warnings/errors/etc. (used make precommit | grep -e '[CRWE][0-9]\{4\}' | awk -F ':' '{print $4}' | sort | uniq -c | sort -n -k 1,1 -r):
 283  C4001
 158  W0613
  98  C0330
  95  W0621
  60  C0326
  56  C0415
  36  W0612
  30  W0221
  30  C0301
  27  E1120
  25  W0212
  21  C0303
  12  C0302
  11  C4002
  10  W0404
   9  W0106
   5  R1722
   5  R0904
   4  W0311
   4  W0107
   4  W0105
   3  W0235
   3  W0104
   3  R1707
   3  R1704
   3  C0121
   2  W1201
   2  W0631
   2  W0143
   2  W0108
   2  W0102
   2  C0305
   1  W1202
   1  W0706
   1  W0702
   1  W0201
   1  W0127
   1  R1723
   1  R1714
   1  R1706
   1  R1702
   1  R0916
   1  R0911
   1  E0601
   1  C0122
  1. C4001 i.e. Invalid string quote ', should be " (invalid-string-quote) is very common in unit tests; the intention is not to satisfy pylint and sacrifice readability, so, as an example this would be a valid way to bypass the error in a unit test while retaining readability:

    # pylint: disable=invalid-string-quote
    assert foobar.to_dict('{"k":2, "l":3.4, "m":true, "n":"str"}') == {"k": 2, "l": 3.4, "m": True, "n": "str"}
    
  2. One suggestion is to split PRs that address one or more of those categories, e.g. one PR to deal with C4001. If the top two offenders result in huge PRs that are difficult to review, it's ok to additionally split them up e.g. one for core Rally files and another for the unittests.

hub-cap added a commit to hub-cap/rally that referenced this issue Dec 17, 2019
hub-cap added a commit to hub-cap/rally that referenced this issue Dec 17, 2019
hub-cap added a commit to hub-cap/rally that referenced this issue Dec 17, 2019
hub-cap added a commit that referenced this issue Dec 18, 2019
hub-cap added a commit that referenced this issue Dec 18, 2019
hub-cap added a commit that referenced this issue Dec 18, 2019
drawlerr added a commit that referenced this issue Dec 18, 2019
hub-cap added a commit to hub-cap/rally that referenced this issue Jan 8, 2020
hub-cap added a commit to hub-cap/rally that referenced this issue Jan 8, 2020
hub-cap added a commit to hub-cap/rally that referenced this issue Jan 9, 2020
This commit will allow us to make progress on the pylint issues we would
like to fix, while allowing users to commit new code that may fail some
of these. What ends up happening is that new code can be merged by other
users, and if precommit is not run in CI, we might accidentally allow a
pylint violation to sneak back in. Removing the pylint check in the
branch/PR that is removing these will allow CI to fail the check so the
user can get feedback if another person has merged code in that violates
the check in the branch/PR.

Relates elastic#838
hub-cap added a commit that referenced this issue Jan 9, 2020
This commit will allow us to make progress on the pylint issues we would
like to fix, while allowing users to commit new code that may fail some
of these. What ends up happening is that new code can be merged by other
users, and if precommit is not run in CI, we might accidentally allow a
pylint violation to sneak back in. Removing the pylint check in the
branch/PR that is removing these will allow CI to fail the check so the
user can get feedback if another person has merged code in that violates
the check in the branch/PR.

Relates #838
@danielmitterdorfer danielmitterdorfer modified the milestones: 1.4.0, 1.4.1 Jan 10, 2020
hub-cap added a commit that referenced this issue Jan 10, 2020
hub-cap added a commit that referenced this issue Jan 13, 2020
@drawlerr drawlerr mentioned this issue Jan 13, 2020
hub-cap added a commit to hub-cap/rally that referenced this issue Oct 8, 2020
hub-cap added a commit to hub-cap/rally that referenced this issue Oct 8, 2020
hub-cap added a commit that referenced this issue Oct 15, 2020
With this commit we reenable the pylint check W0106 which checks
whether an expression not a function call is assigned to nothing.

Relates #838
hub-cap added a commit that referenced this issue Oct 19, 2020
With this commit we reenable the pylint check C4002 which checks
whether triple quotes are properly formatted.

Relates #838
hub-cap added a commit that referenced this issue Oct 19, 2020
With this commit we reenable the pylint check W0221 which checks
whether the correct number of parameters is used in an implemented
or overridden method.

Relates #838
hub-cap added a commit that referenced this issue Oct 21, 2020
With this commit we reenable the pylint check W0612 which checks
whether a variable is defined but unused.

Relates #838
hub-cap added a commit that referenced this issue Oct 21, 2020
With this commit we reenable the pylint check E1120 which checks
whether function calls contain the correct number of arguments.

Relates #838
@danielmitterdorfer danielmitterdorfer modified the milestones: 2.0.2, 2.0.3 Oct 26, 2020
hub-cap added a commit that referenced this issue Nov 5, 2020
With this commit we reenable the pylint check W0404 which checks
whether an import has already been imported.

Relates #838
hub-cap added a commit that referenced this issue Nov 11, 2020
With this commit we reenable the pylint check W0212 which checks
whether protected members are used outside of their classes scope.

Relates #838
danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this issue Nov 18, 2020
With this commit we reenable the pylint warning `assignment-from-none`
which checks whether a `None` return value from a function is assigned
to a variable. We also audited all such issues in the code and disabled
the warning accordingly.

Relates elastic#838
danielmitterdorfer added a commit that referenced this issue Nov 19, 2020
With this commit we reenable the pylint warning `assignment-from-none`
which checks whether a `None` return value from a function is assigned
to a variable. We also audited all such issues in the code and disabled
the warning accordingly.

Relates #838
danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this issue Nov 30, 2020
With this commit we re-enabled the pylint check `R0904` which checks for
too many public methods. We have eliminated the warnings by using
different strategies:

* We have merged too fine-grained methods into more coarse-grained ones.
* We have eliminated some methods that have only been called once.
* We have properly marked protected methods.
* Where appropriate we have disabled the warning (in tests).

Relates elastic#838
danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this issue Nov 30, 2020
With this commit we re-enabled the pylint check `R0904` which checks for
too many public methods. We have eliminated the warnings by using
different strategies:

* We have merged too fine-grained methods into more coarse-grained ones.
* We have eliminated some methods that have been called once or never.
* We have properly marked protected methods.
* Where appropriate we have disabled the warning (in tests).

Relates elastic#838
danielmitterdorfer added a commit that referenced this issue Dec 1, 2020
With this commit we re-enabled the pylint check `R0904` which checks for
too many public methods. We have eliminated the warnings by using
different strategies:

* We have merged too fine-grained methods into more coarse-grained ones.
* We have eliminated some methods that have been called once or never.
* We have properly marked protected methods.
* Where appropriate we have disabled the warning (in tests).

Relates #838
danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this issue Dec 3, 2020
With this commit we re-enable the pylint warning `C0415` which checks
whether modules are imported outside of the toplevel. While we move most
of the imports to toplevel, we keep some imports more targeted, most
notable the `elasticsearch` module so we ensure that the library picks
up Rally's logging configuration properly even in a (multiprocess) actor
system.

Relates elastic#838
danielmitterdorfer added a commit that referenced this issue Dec 7, 2020
With this commit we re-enable the pylint warning `C0415` which checks
whether modules are imported outside of the toplevel. While we move most
of the imports to toplevel, we keep some imports more targeted, most
notable the `elasticsearch` module so we ensure that the library picks
up Rally's logging configuration properly even in a (multiprocess) actor
system.

Relates #838
@DJRickyB DJRickyB modified the milestones: 2.0.3, 2.x Jan 11, 2021
@michaelbaamonde
Copy link
Contributor

I think we can close this one. make precommit returns no errors on master these days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc.
Projects
None yet
Development

No branches or pull requests

6 participants