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 arguments-differ<W0221> linter errors. #1993

Merged
merged 13 commits into from
Sep 16, 2020

Conversation

kevinclark19a
Copy link
Contributor

Description


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

pgombar
pgombar previously approved these changes Aug 26, 2020
@@ -34,7 +34,7 @@ def is_dhcp_enabled(self):
def get_dhcp_pid(self):
return self._get_dhcp_pid(["pidof", "dhcpcd"])

def restart_if(self, ifname): # pylint: disable=W0221
def restart_if(self, ifname, unused_retries=None, unused_wait=None):
Copy link
Member

Choose a reason for hiding this comment

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

we should keep the parameter names consistent, too, in case somebody ever does "restart_if(..., retries=...)

# the original (and, by forwarding the kwargs to the original, will reject any
# option _not_ accepted by the original). Additionally, this method allows us
# to keep the defaults for mount_dvd in one place (the original function) instead
# of having to duplicate it here as well.
Copy link
Member

Choose a reason for hiding this comment

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

But with this signature this function will accept arguments the base function doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of; this function basically immediately passes the kwargs to its super, which would then error out on invalid input.

I realize that that doesn't completely localize the error to this function, though; I felt like this was better than repeating the defaults here as well, and having to keep both up to date. Maybe this isn't the case, though.

Copy link
Member

Choose a reason for hiding this comment

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

ok as is, thanks

pgombar
pgombar previously approved these changes Sep 8, 2020
narrieta
narrieta previously approved these changes Sep 14, 2020
@kevinclark19a kevinclark19a dismissed stale reviews from narrieta and pgombar via 7bfbf3f September 15, 2020 18:04
Copy link
Contributor

@larohra larohra left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -34,7 +34,7 @@ def is_dhcp_enabled(self):
def get_dhcp_pid(self):
return self._get_dhcp_pid(["pidof", "dhcpcd"])

def restart_if(self, ifname): # pylint: disable=W0221
def restart_if(self, ifname, retries=None, wait=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised pylint is not complaining about unused variables now lol

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 was also confused with this, lol. My understanding is that pylint is smart enough to realize that these parameters are used in the base classes' method, that this method is overriding.

@kevinclark19a kevinclark19a merged commit c925c30 into Azure:develop Sep 16, 2020
@kevinclark19a kevinclark19a deleted the LinterArgumentsDiffer branch September 16, 2020 17:28
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.

4 participants