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.

Sign up for GitHub

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

[WIP] Format code with black #5425

Closed
wants to merge 4 commits into from
Closed

[WIP] Format code with black #5425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58c283e
Add a mechanism to format code using black
dstufft May 18, 2018
49de4dc
Temporarily remove isort to demo black error
dstufft Jun 6, 2018
0f54f09
Manual tweak to fix linting error
dstufft Jun 7, 2018
d276d13
Format with black
dstufft Jun 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ matrix:
- env: TOXENV=docs
- env: TOXENV=lint-py2
- env: TOXENV=lint-py3
python: 3.6
- env: TOXENV=mypy
- env: TOXENV=packaging
# PyPy jobs start first -- they are the slowest
Expand Down
27 changes: 5 additions & 22 deletions docs/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
'collapsiblesidebar': True,
'externalrefs': True,
'navigation_depth': 3,
'issues_url': 'https://github.com/pypa/pip/issues'
'issues_url': 'https://github.com/pypa/pip/issues',
}

# Add any paths that contain custom themes here, relative to this directory.
Expand Down Expand Up @@ -177,10 +177,7 @@
smart_quotes = False

# Custom sidebar templates, maps document names to template names.
html_sidebars = {
'**': ['localtoc.html', 'relations.html'],
'index': ['localtoc.html']
}
html_sidebars = {'**': ['localtoc.html', 'relations.html'], 'index': ['localtoc.html']}

# Additional templates that should be rendered to pages, maps page names to
# template names.
Expand Down Expand Up @@ -221,13 +218,7 @@
# Grouping the document tree into LaTeX files. List of tuples
# (source start file, target name, title, author, documentclass [howto/manual])
latex_documents = [
(
'index',
'pip.tex',
u'pip Documentation',
u'pip developers',
'manual',
),
('index', 'pip.tex', u'pip Documentation', u'pip developers', 'manual')
]

# The name of an image file (relative to this directory) to place at the top of
Expand All @@ -251,22 +242,14 @@

# List of manual pages generated
man_pages = [
(
'man/pip',
'pip',
u'package manager for Python packages',
u'pip developers',
1
)
('man/pip', 'pip', u'package manager for Python packages', u'pip developers', 1)
]

# Here, we crawl the entire man/commands/ directory and list every file with
# appropriate name and details
for fname in glob.glob('man/commands/*.rst'):
fname_base = fname[:-4]
outname = 'pip-' + fname_base[13:]
description = u'description of {} command'.format(
outname.replace('-', ' ')
)
description = u'description of {} command'.format(outname.replace('-', ' '))

man_pages.append((fname_base, outname, description, u'pip developers', 1))
14 changes: 3 additions & 11 deletions docs/pipext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ def run(self):


class PipOptions(rst.Directive):

def _format_option(self, option, cmd_name=None):
if cmd_name:
bookmark_line = ".. _`%s_%s`:" % (cmd_name, option._long_opts[0])
Expand Down Expand Up @@ -77,27 +76,20 @@ def run(self):

class PipGeneralOptions(PipOptions):
def process_options(self):
self._format_options(
[o() for o in cmdoptions.general_group['options']]
)
self._format_options([o() for o in cmdoptions.general_group['options']])


class PipIndexOptions(PipOptions):
def process_options(self):
self._format_options(
[o() for o in cmdoptions.index_group['options']]
)
self._format_options([o() for o in cmdoptions.index_group['options']])


class PipCommandOptions(PipOptions):
required_arguments = 1

def process_options(self):
cmd = commands[self.arguments[0]]()
self._format_options(
cmd.parser.option_groups[0].option_list,
cmd_name=cmd.name,
)
self._format_options(cmd.parser.option_groups[0].option_list, cmd_name=cmd.name)


def setup(app):
Expand Down
7 changes: 5 additions & 2 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,21 @@
skip =
_vendor
__main__.py
multi_line_output = 5
multi_line_output = 3
known_third_party =
pip._vendor
known_first_party =
pip
tests
default_section = THIRDPARTY
include_trailing_comma = true
force_grid_wrap = 0
line_length = 88

[flake8]
max-line-length = 88
# Ignoring unused imports since mypy would warn of that.
ignore = F401
ignore = F401,W503,E203
exclude = .tox,.idea,*.egg,build,_vendor,data
select = E,W,F

Expand Down
17 changes: 3 additions & 14 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ def read(*parts):

def find_version(*file_paths):
version_file = read(*file_paths)
version_match = re.search(
r"^__version__ = ['\"]([^'\"]*)['\"]",
version_file,
re.M,
)
version_match = re.search(r"^__version__ = ['\"]([^'\"]*)['\"]", version_file, re.M)
if version_match:
return version_match.group(1)

Expand All @@ -36,7 +32,6 @@ def find_version(*file_paths):
version=find_version("src", "pip", "__init__.py"),
description="The PyPA recommended tool for installing Python packages.",
long_description=long_description,

license='MIT',
classifiers=[
"Development Status :: 5 - Production/Stable",
Expand All @@ -55,15 +50,10 @@ def find_version(*file_paths):
],
url='https://pip.pypa.io/',
keywords='distutils easy_install egg setuptools wheel virtualenv',

author='The pip developers',
author_email='pypa-dev@groups.google.com',

package_dir={"": "src"},
packages=find_packages(
where="src",
exclude=["contrib", "docs", "tests*", "tasks"],
),
packages=find_packages(where="src", exclude=["contrib", "docs", "tests*", "tasks"]),
package_data={
"pip._vendor.certifi": ["*.pem"],
"pip._vendor.requests": ["*.pem"],
Expand All @@ -75,9 +65,8 @@ def find_version(*file_paths):
"pip=pip._internal:main",
"pip%s=pip._internal:main" % sys.version_info[:1],
"pip%s.%s=pip._internal:main" % sys.version_info[:2],
],
]
},

zip_safe=False,
python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*',
)
19 changes: 10 additions & 9 deletions src/pip/_internal/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
# in the DEP-8 tests, so just suppress the warning. pdb tells me this has to
# be done before the import of pip.vcs.
from pip._vendor.urllib3.exceptions import DependencyWarning

warnings.filterwarnings("ignore", category=DependencyWarning) # noqa

# We want to inject the use of SecureTransport as early as possible so that any
Expand All @@ -44,9 +45,7 @@
from pip._internal.utils.misc import get_installed_distributions, get_prog
from pip._internal.utils import deprecation
from pip._internal.vcs import git, mercurial, subversion, bazaar # noqa
from pip._internal.baseparser import (
ConfigOptionParser, UpdatingDefaultsHelpFormatter,
)
from pip._internal.baseparser import ConfigOptionParser, UpdatingDefaultsHelpFormatter
from pip._internal.commands import get_summaries, get_similar_commands
from pip._internal.commands import commands_dict
from pip._vendor.urllib3.exceptions import InsecureRequestWarning
Expand Down Expand Up @@ -88,10 +87,10 @@ def autocomplete():
if subcommand_name == 'help':
sys.exit(1)
# special case: list locally installed dists for show and uninstall
should_list_installed = (
subcommand_name in ['show', 'uninstall'] and
not current.startswith('-')
)
should_list_installed = subcommand_name in [
'show',
'uninstall',
] and not current.startswith('-')
if should_list_installed:
installed = []
lc = current.lower()
Expand All @@ -112,7 +111,7 @@ def autocomplete():
options.append((opt_str, opt.nargs))

# filter out previously specified options from available options
prev_opts = [x.split('=')[0] for x in cwords[1:cword - 1]]
prev_opts = [x.split('=')[0] for x in cwords[1 : cword - 1]]
Copy link
Member

Choose a reason for hiding this comment

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

This is another style rule I dislike. Colons in slices are not operators, and shouldn't be formatted that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually not what PEP 8 says! (I was surprised too), PEP 8 says:

However, in a slice the colon acts like a binary operator, and should have equal amounts on either side (treating it as the operator with the lowest priority). In an extended slice, both colons must have the same amount of spacing applied. Exception: when a slice parameter is omitted, the space is omitted.

Yes:

ham[1:9], ham[1:9:3], ham[:9:3], ham[1::3], ham[1:9:]
ham[lower:upper], ham[lower:upper:], ham[lower::step]
ham[lower+offset : upper+offset]
ham[: upper_fn(x) : step_fn(x)], ham[:: step_fn(x)]
ham[lower + offset : upper + offset]

No:

ham[lower + offset:upper + offset]
ham[1: 9], ham[1 :9], ham[1:9 :3]
ham[lower : : upper]
ham[ : upper]

In this case, the original formatted was basically exactly one of the No examples:

ham[lower + offset:upper + offset]

Copy link
Member

@pfmoore pfmoore Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I never said I strictly follow PEP 8 :-)

But our current checks allow this, and I personally prefer the current layout, is all I was saying. So black's stricter approach (even if it is PEP 8 conformant) isn't one I like.

What I dislike about automatic application of style rules (and especially when it's done in the name of PEP 8) is that it ignores the overriding point in PEP 8:

However, know when to be inconsistent -- sometimes style guide recommendations just aren't applicable. When in doubt, use your best judgment.

(And yes, I know fmt: off is available for this, but use of that is much more likely to trigger a subjective readability debate than a readable, not 100% compliant, but passes the checks piece of code).

options = [(x, v) for (x, v) in options if x not in prev_opts]
# filter options by current input
options = [(k, v) for k, v in options if k.startswith(current)]
Expand Down Expand Up @@ -151,7 +150,9 @@ def create_main_parser():

pip_pkg_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
parser.version = 'pip %s from %s (python %s)' % (
__version__, pip_pkg_dir, sys.version[:3],
__version__,
pip_pkg_dir,
sys.version[:3],
)

# add the general options
Expand Down
Loading