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

Blacken salt (Implements SEP 15) #55765

Merged
merged 3 commits into from
Apr 6, 2020
Merged

Blacken salt (Implements SEP 15) #55765

merged 3 commits into from
Apr 6, 2020

Conversation

waynew
Copy link
Contributor

@waynew waynew commented Dec 30, 2019

What does this PR do?

What it says on the tin. Implements SEP 15 - at least the black portion of it. Apparently isort will cause a bunch of issues with our linting, because of the number of lint comments around imports.

What issues does this PR fix or reference?

SEP 15

Previous Behavior

Salt's code was painted many different colors of bikeshed

New Behavior

Salt's code is now a lovely shade of Black

Tests written?

No (code should not have been changed)

Commits signed with GPG?

Yes

@waynew waynew requested a review from a team as a code owner December 30, 2019 22:17
@ghost ghost requested a review from Ch3LL December 30, 2019 22:17
@rares-pop
Copy link
Contributor

This is pretty cool! There seem to be some config issues, or really black issues (PyCQA/pycodestyle#373), but things look pretty good!

Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

In the pre-commit config dotfile on Salt's repo, let's add something like:

- repo: https://github.com/ambv/black
  rev: stable
  hooks:
  - id: black
    args: [--line-length, '120', .]

Ajust args to those you used when blackening Salt.

@waynew
Copy link
Contributor Author

waynew commented Jan 3, 2020

Well, I'm sure there will be a few more merge conflicts, but this should be much further down the path. Also I fixed all the lint issues that I had, and added black to pre-commit 🎉

Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

Also, any files under salt/ext should be ignored by black. These are vendored modules which we should keep as intact as possible.

.testing.pylintrc Outdated Show resolved Hide resolved
@waynew waynew force-pushed the blacken-salt branch 2 times, most recently from 4f68b18 to 7c84877 Compare February 6, 2020 20:33
@mattp-
Copy link
Contributor

mattp- commented Feb 10, 2020

is it worth holding off on this until the PR queue from other branches has been considered 'resolved' and handled?

@waynew
Copy link
Contributor Author

waynew commented Feb 10, 2020

@mattp- I would say no. Rebasing and blackening the other PRs should avoid merge conflicts there. Also, I'm pretty sure that most or all the PRs that had tests have been merged.

Meanwhile, we'll have any new development unable to take advantage of black.

If there was a weeks worth of effort or even maybe a month I might feel otherwise, but there's still quite a lot of work to do in the master port effort. We're going to have to deal with merge conflicts and other issues anyway - at least if the code is blackened first we can avoid any code formatting issues there.

@waynew waynew force-pushed the blacken-salt branch 2 times, most recently from 4607548 to 75ff102 Compare February 11, 2020 02:54
@sagetherage sagetherage added the ZRelease-Sodium retired label label Mar 19, 2020
@sagetherage sagetherage added this to Unassigned in Core Open PRs via automation Mar 19, 2020
@sagetherage sagetherage added this to In progress in Sodium via automation Mar 19, 2020
@waynew waynew changed the title Blacken salt (Implements the Black portion of SEP 15) Blacken salt (Implements SEP 15) Apr 3, 2020
Core Open PRs automation moved this from Unassigned to In progress Apr 3, 2020
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

Code style: black

Of course I didn't review all files, yet, but there are already some things to address.

.pre-commit-config.yaml Show resolved Hide resolved
.pylintrc Outdated

# Add files or directories matching the regex patterns to the blacklist. The
# regex matches against base names, not paths.
ignore-patterns=
ignore-patterns=salt/ext/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the comment says, these are basename regex patterns, not paths.
This won't do what you want.

.pylintrc Show resolved Hide resolved
doc/.scripts/setup-transifex-config Show resolved Hide resolved
.isort.cfg Show resolved Hide resolved
)
sys.stderr.flush()
exit(1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea was to write to stderr even before trying to import nox.
We can wrap this code block with fmt: on/off.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this back up?

pkg/rpm/build.py Outdated
parser.add_argument('buildid',
help='The build id to use i.e. the bit after the salt version in the package name',
)
import salt.version
Copy link
Collaborator

Choose a reason for hiding this comment

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

This salt.version import should probably go back to where it was, see the sys.path.append before it?

from salt._logging.mixins import LoggingMixinMeta, NewStyleClassMixin
from salt.exceptions import LoggingRuntimeError
from salt.utils.ctx import RequestContext
from salt.utils.textformat import TextFormat
Copy link
Collaborator

Choose a reason for hiding this comment

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

These imports need to go back to where they were. See the comment above where we define the new log levels.

E203 is *not* pep8 compliant, pylint.
@s0undt3ch
Copy link
Collaborator

Restarted CentOS 8, if that build passes, let's merge.
We can address my review comments at a latter stage.
Let's not consider them blockers.

@waynew
Copy link
Contributor Author

waynew commented Apr 6, 2020

@s0undt3ch https://jenkinsci.saltstack.com/job/pr-centos8-py3/job/PR-55765/15/ \o/

Codecov is complaining, and uh... I think I broke it 🙃 Tried to go to codecov to dismiss that change. but it was very unhappy 😝

@s0undt3ch
Copy link
Collaborator

Ignore codecov

@dwoz dwoz merged commit a670b4a into saltstack:master Apr 6, 2020
Sodium automation moved this from In progress to Done Apr 6, 2020
Core Open PRs automation moved this from In progress to Done Apr 6, 2020
@s0undt3ch
Copy link
Collaborator

🎆

@waynew
Copy link
Contributor Author

waynew commented Apr 7, 2020

🖤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
No open projects
Sodium
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants