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

Linter updates #8803

Closed
wants to merge 9 commits into from
Closed

Linter updates #8803

wants to merge 9 commits into from

Conversation

pradyunsg
Copy link
Member

Closes #8543

@pradyunsg pradyunsg added the type: maintenance Related to Development and Maintenance Processes label Aug 25, 2020
Comment on lines +84 to 86
textwrap.dedent(
"""
import os, site, sys
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to reindent the code literal here.

Also, could we please have 79-column limit?

Also known as fixing flake8's E231 errors. :)
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

lot of files to view :). Would be crazy if we try to create batch of files?

@@ -76,13 +77,14 @@ def test(session):
run_with_protected_pip(
session,
"wheel",
"-w", LOCATIONS["common-wheels"],
"-r", REQUIREMENTS["common-wheels"],
"-w",
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO is more redeable left as orginal. What is your opinion?

msg = (
"Re-using existing common-wheels at {}."
.format(LOCATIONS["common-wheels"])
msg = "Re-using existing common-wheels at {}.".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

why not change to f-string?

Copy link
Contributor

Choose a reason for hiding this comment

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

f-string is only available in Python 3.6+.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 26, 2020
@McSinyx
Copy link
Contributor

McSinyx commented Aug 26, 2020

I know that the linters aren't complaining but I still want to not see inline trailing commas:

$ ack ',\)' src/pip/_internal/
src/pip/_internal/commands/download.py
73:        index_opts = cmdoptions.make_option_group(cmdoptions.index_group, self.parser,)

src/pip/_internal/commands/wheel.py
107:        index_opts = cmdoptions.make_option_group(cmdoptions.index_group, self.parser,)

src/pip/_internal/commands/install.py
224:        index_opts = cmdoptions.make_option_group(cmdoptions.index_group, self.parser,)
556:                ).format(name=project_name, version=version, requirement=dependency[1],)

src/pip/_internal/commands/list.py
246:        packages = sorted(packages, key=lambda dist: dist.project_name.lower(),)

src/pip/_internal/commands/uninstall.py
65:            req = install_req_from_line(name, isolated=options.isolated_mode,)

src/pip/_internal/index/package_finder.py
213:            version = _extract_version_from_fragment(egg_info, self._canonical_name,)
792:            package_links = self.evaluate_links(link_evaluator, links=page_links,)
902:                    sorted({str(c.version) for c in cand_iter}, key=parse_version,)

src/pip/_internal/index/collector.py
375:        link = _create_link_from_element(anchor, page_url=url, base_url=base_url,)
635:        search_scope = SearchScope.create(find_links=find_links, index_urls=index_urls,)
636:        link_collector = LinkCollector(session=session, search_scope=search_scope,)
660:        fl_file_loc, fl_url_loc = group_locations(self.find_links, expand_dir=True,)

src/pip/_internal/resolution/legacy/resolver.py
393:            sub_install_req = self._make_install_req(str(subreq), req_to_install,)

src/pip/_internal/resolution/resolvelib/provider.py
127:        constraint = self._constraints.get(requirements[0].name, SpecifierSet(),)

src/pip/_internal/models/direct_url.py
224:        res = _filter_none(url=self.redacted_url, subdirectory=self.subdirectory,)

src/pip/_internal/models/search_scope.py
65:        return cls(find_links=built_find_links, index_urls=index_urls,)

src/pip/_internal/req/req_file.py
264:        search_scope = SearchScope(find_links=find_links, index_urls=index_urls,)
359:                    req_path = os.path.join(os.path.dirname(filename), req_path,)
361:                for inner_line in self._parse_and_recurse(req_path, nested_constraint,):

src/pip/_internal/req/req_install.py
90:    return dist_cls(base_dir, project_name=dist_name, metadata=metadata,)
276:            extras_requested = ("",)
521:        return generate_metadata(build_env=self.build_env, backend=self.pep517_backend,)

src/pip/_internal/cli/base_command.py
80:        gen_opts = cmdoptions.make_option_group(cmdoptions.general_group, self.parser,)

src/pip/_internal/cli/cmdoptions.py
563:        msg = "invalid --python-version value: {!r}: {}".format(value, error_msg,)

src/pip/_internal/cache.py
232:                (wheel.support_index_min(supported_tags), wheel_name, wheel_dir,)

src/pip/_internal/operations/freeze.py
125:                        line_req = install_req_from_editable(line, isolated=isolated,)

src/pip/_internal/operations/build/wheel_legacy.py
93:            output = call_subprocess(wheel_args, cwd=source_dir, spinner=spinner,)

src/pip/_internal/operations/prepare.py
245:        file = get_http_url(link, download, download_dir, hashes=hashes,)

src/pip/_internal/locations.py
147:            scheme["headers"] = os.path.join(root, path_no_drive[1:],)

src/pip/_internal/utils/compatibility_tags.py
94:        return (int(version[0]),)
144:            cpython_tags(python_version=python_version, abis=abis, platforms=platforms,)
148:            generic_tags(interpreter=interpreter, abis=abis, platforms=platforms,)

src/pip/_internal/utils/encoding.py
39:    return data.decode(locale.getpreferredencoding(False) or sys.getdefaultencoding(),)

src/pip/_internal/network/session.py
134:        libc = dict(filter(lambda x: x[1], zip(["lib", "version"], libc_ver()),))
# perhaps: libc = {x: y for x, y in zip(["lib", "version"], libc_ver()) if y}

src/pip/_internal/network/auth.py
117:        url, netloc, url_user_password = split_auth_netloc_from_url(original_url,)

src/pip/_internal/vcs/subversion.py
164:                xml = cls.run_command(["info", "--xml", location],)

src/pip/_internal/vcs/bazaar.py
106:        revision = cls.run_command(["revno"], cwd=location,)

src/pip/_internal/vcs/git.py
107:        output = cls.run_command(args, extra_ok_returncodes=(1,), cwd=location,)
239:                    cmd_args = make_command("checkout", "-q", rev_options.to_args(),)
314:        current_rev = cls.run_command(["rev-parse", rev], cwd=location,)

src/pip/_internal/vcs/mercurial.py
141:            r = cls.run_command(["root"], cwd=location, log_failed_cmd=False,)

@pfmoore
Copy link
Member

pfmoore commented Aug 26, 2020

I know that the linters aren't complaining but I still want to not see inline trailing commas

100% agreed. Looks like this is a case where black has reformatted stuff, and we now need a manual tidying up exercise to fix stuff that it's left looking ugly.

I'm strongly against these - let's fix them, even if it means manually going through and removing the extra commas. And if there's any linter setting that flags these, then we should enable it so we don't get more of them creeping into the codebase.

(BTW, I'm surprised that a PR entitled "linter updates" also includes enabling black - last time the topic came up, it was quite controversial, so I'd have preferred it to be made clearer that it was part of this PR. I nearly missed that it was going on. I don't object to it - subject to the above - but I just think it's better to be more explicit over it).

@pradyunsg
Copy link
Member Author

Well, this was supposed to be a WIP. :)

@pfmoore
Copy link
Member

pfmoore commented Aug 26, 2020

Sorry - hadn't realised. And if I'd checked the linked issue I would have noticed that black was mentioned, so that's to an extent my bad.

But I would like those trailing commas sorted - if you intended to do so anyway, that's cool. (Sometimes it's hard to know when comments are welcome and when they are premature, and I have a tendency to comment "now, in case I forget", which isn't always the best idea 😉)

@pradyunsg
Copy link
Member Author

You have no reason to apologize for a goof-up caused by me. :)

I know that the linters aren't complaining but I still want to not see inline trailing commas:

I noticed. What I'm wondering is... why are the linters not complaining?

@pradyunsg pradyunsg marked this pull request as draft August 26, 2020 20:09
@uranusjr
Copy link
Member

Good news: Black just made a release that supposedly would fix the trailing comma issue!

@pradyunsg
Copy link
Member Author

Closing, since I'll make a new PR based off current master. :)

@pradyunsg pradyunsg closed this Sep 23, 2020
This was referenced Sep 23, 2020
@pradyunsg pradyunsg added this to the The Blackening milestone Sep 23, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopting black (and linter updates!)
6 participants