Skip to content

Fix make style-check errors#1000

Merged
TheRealFalcon merged 1 commit into
canonical:mainfrom
sshedi:misc-fixes
Sep 1, 2021
Merged

Fix make style-check errors#1000
TheRealFalcon merged 1 commit into
canonical:mainfrom
sshedi:misc-fixes

Conversation

@sshedi
Copy link
Copy Markdown
Contributor

@sshedi sshedi commented Aug 25, 2021

  Fix make style-check errors

Using flake8 inplace of pyflakes
Renamed run-pyflakes -> run-flake8
Changed target name to flake8 in Makefile

With pyflakes we can't suppress warnings/errors in few required places.
flake8 is flexible in that regard. Hence using flake8 seems to be a
better choice here.

flake8 does the job of pep8 anyway.
So, removed pep8 target from Makefile along with tools/run-pep8 script.

Included setup.py in flake8 checks

Signed-off-by: Shreenidhi Shedi sshedi@vmware.com

Proposed Commit Message

 Fix make style-check errors

Using flake8 inplace of pyflakes
Renamed run-pyflakes -> run-flake8
Changed target name to flake8 in Makefile

With pyflakes we can't suppress warnings/errors in few required places.
flake8 is flexible in that regard. Hence using flake8 seems to be a
better choice here.

flake8 does the job of pep8 anyway.
So, removed pep8 target from Makefile along with tools/run-pep8 script.

Included setup.py in flake8 checks

Test Steps

make style-check; echo $?

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

  • The run-pep8 stuff can be removed entirely. It's functionality is entirely covered by flake8.
  • How are you running linting such that you're getting "F401" on the typing imports? They're used in the type annotation comments which doesn't trigger a warning for me.
  • Can we leave W503 and W504 in and revert the changes for them back? These were added intentionally and I see no reason to pull them out...especially since the guidance has reversed in recent years.

Comment thread cloudinit/cmd/main.py Outdated
Comment on lines +41 to +44
from cloudinit import patcher
patcher.patch_logging()
from cloudinit import log as logging # noqa: E402

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's not move this. I want the patch to happen before other cloud-init imports.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, understood. My question is - can we do it in some other place and avoid this warning?

Like in /usr/bin/cloud-init before anything starts?

Comment thread cloudinit/cmd/devel/hotplug_hook.py Outdated
Comment thread tests/integration_tests/instances.py
@sshedi
Copy link
Copy Markdown
Contributor Author

sshedi commented Aug 31, 2021

I ran checks on Ubuntu-16.04 and it reported these errors. Are there any plans to stop running CI tests on Ubuntu 16.04?
The reason why I'm using 16.04 is, it behaves different compared to other versions. If tests work on 16.04, then most probably it will work on higher versions. I had a hard time while writing pytests for some modules because of dictionary ordering issues.

@sshedi
Copy link
Copy Markdown
Contributor Author

sshedi commented Aug 31, 2021

I will remove run-pep8 target and corresponding script.Thanks.

You want me to keep the ignore W503 & W504 in tox.ini as well? If guidance is reversed, it seems redundant.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Are there any plans to stop running CI tests on Ubuntu 16.04?

16.04 no longer receives standard support, so from an upstream perspective, we're not concerned about breaking things in 16.04. Practically, we still run it in our CI because it has the lowest version of python that we support (3.5), so it's an easy way to test our 3.5 dependencies.

You want me to keep the ignore W503 & W504 in tox.ini as well?

Correct.

Using flake8 inplace of pyflakes
Renamed run-pyflakes -> run-flake8
Changed target name to flake8 in Makefile

With pyflakes we can't suppress warnings/errors in few required places.
flake8 is flexible in that regard. Hence using flake8 seems to be a
better choice here.

flake8 does the job of pep8 anyway.
So, removed pep8 target from Makefile along with tools/run-pep8 script.

Included setup.py in flake8 checks

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

2 participants