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

Ruff rule to remove six #11091

Closed
inoa-jboliveira opened this issue Apr 22, 2024 · 9 comments
Closed

Ruff rule to remove six #11091

inoa-jboliveira opened this issue Apr 22, 2024 · 9 comments
Labels
needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule

Comments

@inoa-jboliveira
Copy link

inoa-jboliveira commented Apr 22, 2024

Proposal: creation of rules to remove usages of the six module, replacing with the Python 3 only equivalent. This would align with the UP rules, but I don't know if they would be allowed in there or simply into the RUF code. The idea is to have fixable rules for the most common features of six and help people migrate with more confidence. There should also exist a catch-all rule just checking if six was imported.

  • UP900 Deprecate six (not fixable) .
    • E.g. import six -> "Usage of six is discouraged on applications not supporting Python 2"
  • UP901 Replace types (e.g. six.string_types, six.text_type, six.binary_type, six.integer_types) with corresponding Python 3 types.
    • E.g.: isinstance(foo, six.string_types) -> isintance(foo, (str,))
    • Note: arguably this is the most used feature of six
  • UP902 Replace six.iter* with the corresponding method.
    • E.g.: for k, v in six.iteritems(foo): -> for k, v in foo.items():
  • UP903 Replace six.print_ with print.
    • E.g.: six.print_("hello") -> print("hello")
  • UP904 Replace six.PY2, PY3, ... with direct checks.
    • E.g.: if six.PY3: -> if sys.version_info[0] == 3:
    • Note: this is a quite important one because six.PY3 is a source of bugs -- see YTT202
    • Could also warn the user that the check is no longer be necessary since all code is running on Python 3
@charliermarsh
Copy link
Member

There's some of this in the UP rules already (like UP029 removes six.callable and others that are now builtins), but I think I intentionally cut scope on other six rules because I wanted to avoid adding and maintaining complex rules that would only be relevant for one-time 2-to-3 transitions (as opposed to rules that are useful for ongoing Python 3 development).

@charliermarsh
Copy link
Member

I'm open to adding some of these selectively under the UP category but it's a bit of a balance -- I don't want to add and maintain a bunch of complex six rules since the value is diminishing over time.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Apr 23, 2024
@MichaReiser
Copy link
Member

I share the sentiment with @charliermarsh. If the primary target group is python 2 users, then we should not implement the rule according to our rule acceptance guidelines.

UP900 can be achieved by using banned-api

@inoa-jboliveira
Copy link
Author

inoa-jboliveira commented Apr 23, 2024

Hi @charliermarsh and @MichaReiser, thank you for the replies.

The point here is not serve people upgrading from python 2, it is to drop the old compatibility layer many projects kept for a long time even now supporting most modern python versions only.

Same thing with UP025 where you replace "u" prefix of strings which is leftover from years of legacy support.

I have a repo with over 1M LOC of python and it took a while to remove everything, but then I noticed many dependencies still imported six even if they no longer supported python 2.

This means having these rules could speed up projects from dropping this layer (again same purpose as many of the UP rules).

If you feel this doesn't bring enough benefit, you may close the issue

@eli-schwartz
Copy link
Contributor

Related:

#2049 (comment)
#2332

@eli-schwartz
Copy link
Contributor

If it's possible to reimplement via more generic rewriting methods that didn't exist at the time, I think that would be quite convenient as IMO the UP rules are mostly a matter for oneshot actions, and six isn't particularly special. I think the only real reason in the past for not covering these rules effectively boiled down to implementation complexity and no one available to do the work?

@jamesliu4c
Copy link

What about just the first rule?

UP900 Deprecate six (not fixable) .
E.g. import six -> "Usage of six is discouraged on applications not supporting Python 2"

The only things that have to be checked are import six and from six import.

This would be a minimal amount of work to implement and maintain.

It would also be high impact. I implemented this as a Pylint rule in my org. It was an enormous success. We were two years past a migration to Python 3 and had a bunch of six code lying around. We also had a rule that required lint to pass to merge, so within a few months, six was gone from our codebase.

@inoa-jboliveira
Copy link
Author

inoa-jboliveira commented Dec 2, 2024

@jamesliu4c

The only things that have to be checked are import six and from six import.

For that case alone, if you can use TID251 and configure six on banned-api for pyproject.toml

Sometimes I use TID253 if the ban is only on top level imports

By the way, I now agree with the ruff team here as there are other ways to block six and there is low value in maintaining these rules.

IMO this Issue can be closed

@jamesliu4c
Copy link

Oh, that's great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

6 participants