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

Enabled linter for azurelinuxagent.daemon. #1979

Merged
merged 5 commits into from Aug 17, 2020

Conversation

kevinclark19a
Copy link
Contributor

@kevinclark19a kevinclark19a commented Aug 11, 2020

Description

5th of 7 PRs enabling linter in Travis.

Error Codes can be found here: http://pylint.pycqa.org/en/latest/technical_reference/features.html


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

@codecov
Copy link

codecov bot commented Aug 11, 2020

Codecov Report

Merging #1979 into develop will not change coverage.
The diff coverage is 71.87%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1979   +/-   ##
========================================
  Coverage    71.14%   71.14%           
========================================
  Files           86       86           
  Lines        12285    12285           
  Branches      1728     1728           
========================================
  Hits          8740     8740           
  Misses        3159     3159           
  Partials       386      386           
Impacted Files Coverage Δ
azurelinuxagent/daemon/resourcedisk/openbsd.py 18.33% <0.00%> (ø)
azurelinuxagent/daemon/resourcedisk/openwrt.py 16.66% <50.00%> (ø)
azurelinuxagent/daemon/resourcedisk/freebsd.py 12.87% <66.66%> (ø)
azurelinuxagent/daemon/resourcedisk/default.py 38.27% <72.72%> (ø)
azurelinuxagent/daemon/main.py 72.16% <80.00%> (ø)
azurelinuxagent/daemon/resourcedisk/factory.py 57.14% <100.00%> (ø)
azurelinuxagent/daemon/scvmm.py 82.92% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d2ac9f...5b1e61d. Read the comment docs.

@narrieta
Copy link
Member

Error Codes can be found here: http://pylint.pycqa.org/en/latest/technical_reference/features.html

Can we add a table of the errors for the agent, maybe as comments in one of the configuration files?

2.7.pylintrc Outdated

[MESSAGES CONTROL]

disable=bad-option-value,misplaced-comparison-constant,no-self-use,consider-iterating-dictionary,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a short description of each suppresion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even a link pointing to the suppression docs with the description if possible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinclark19a - my main goal is to make it easy to fix the code... i'm thinking we would come to this file, read the description of the issues, pick up one, highlight the code or symbol, do a global search on the code using the highlighted text, and then go fix each instance

old-style-class, deprecated-lambda,
duplicate-code #TODO:

[SIMILARITIES]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this section do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This are fine tuning options for the duplicate-code error message. It determines under which conditions that error will be reported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, may be a good idea to add a comment that this is for duplicate-code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to just link to the pylintrc options documentation? That would have the description of all of the sections, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want, you can do that.

i read the comments below, e.g. "# Minimum lines number of a similarity." and I had no idea what that was. A short comment on the section, e.g. "Settings for duplicate-code" would allow the reader to get a quick idea of what this is about without having to reach out to the documentation.

3.6.pylintrc Outdated

[MESSAGES CONTROL]

disable=misplaced-comparison-constant,no-self-use,consider-iterating-dictionary,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we list the suppressions in alphabetical order, one per line? it is hard to compare this list with the list for 2.7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Do the suppressions map one to one to error codes? If so, can we add those as comments here so it's easier to refer to the error codes in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Working on this!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it's easier to refer to the error codes in the code?

Can we either put the error codes here or put the short names in the supression comments in the code just to keep it consistent? This ofcourse in addition to having the a mapped description with maybe the public link for each suppression too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put both the symbol and the error code in the comments here.

@@ -3,7 +3,8 @@ os: linux
dist: xenial
language: python
env:
- NOSEOPTS="--verbose --with-timer" SETUPOPTS=""
- >-
NOSEOPTS="--verbose --with-timer" SETUPOPTS=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove all the flake8 stuff

Copy link
Contributor

@pgombar pgombar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Excited to enable linting once all of your changes go in 😎

@kevinclark19a kevinclark19a merged commit e004bd0 into Azure:develop Aug 17, 2020
@kevinclark19a kevinclark19a deleted the LinterEnabledDaemon branch August 17, 2020 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants