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

Update to latest meta/config templates and drop Python 3.7. #69

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

dataflake
Copy link
Member

No description provided.

@dataflake dataflake requested review from icemac and tseaver August 27, 2024 15:45
@dataflake
Copy link
Member Author

This is an odd one. Running pyupgrade will break this package so I had to disable it manually, there's no way to say "don't touch this line" or "don't touch this file". The diff below shows what it wants to do, but that breaks all unit tests. I copied a sample traceback at the bottom.

+++ b/src/zope/proxy/__init__.py
@@ -107,7 +107,7 @@ class AbstractPyProxyBase:

     def __new__(cls, value=None):
         # Some subclasses (zope.security.proxy) fail to pass the object
-        inst = super(AbstractPyProxyBase, cls).__new__(cls)
+        inst = super().__new__(cls)
         inst._wrapped = value
         return inst

@@ -197,7 +197,7 @@ class AbstractPyProxyBase:

     def __setattr__(self, name, value):
         if name == '_wrapped':
-            return super(AbstractPyProxyBase, self).__setattr__(name, value)
+            return super().__setattr__(name, value)

         # First, look for descriptors in this object's type
         type_self = type(self)
Traceback (most recent call last):
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/bin/zope-testrunner", line 8, in <module>
    sys.exit(run())
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/testrunner/__init__.py", line 31, in run
    failed = run_internal(defaults, args, script_parts=script_parts, cwd=cwd,
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/testrunner/__init__.py", line 52, in run_internal
    from zope.testrunner.runner import Runner
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/testrunner/runner.py", line 48, in <module>
    import zope.testrunner.tb_format
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/testrunner/tb_format.py", line 20, in <module>
    import zope.exceptions.exceptionformatter
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/exceptions/__init__.py", line 37, in <module>
    import zope.security  # noqa: suppress unused import warning from flake8
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/security/__init__.py", line 20, in <module>
    import zope.security.decorator
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/security/decorator.py", line 24, in <module>
    from zope.security.checker import CombinedChecker
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/security/checker.py", line 356, in <module>
    CheckerPublic = Proxy(CheckerPublic, Checker(d))  # XXX uses CheckerPy
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/security/proxy.py", line 83, in __new__
    inst = super().__new__(cls)
  File "/Users/jens/src/zope/zope.proxy/.tox/py38-pure/lib/python3.8/site-packages/zope/proxy/__init__.py", line 110, in __new__
    inst = super().__new__(cls)
TypeError: super(type, obj): obj must be an instance or subtype of type

@dataflake dataflake self-assigned this Aug 28, 2024
@dataflake dataflake merged commit 47e2890 into master Aug 29, 2024
53 checks passed
@dataflake dataflake deleted the config-with-c-code-template-4eec7859 branch August 29, 2024 04:55
@icemac
Copy link
Member

icemac commented Aug 30, 2024

I cannot reproduce the problem locally:

class A:
    def __new__(cls):
        pass


class B(A):
    def __new__(cls):
        super().__new__(cls)


b = B()

Does not fail. Or do I do something wrong here?

@dataflake
Copy link
Member Author

I did not test this manually and did not see a reason to. If I apply all changes pyupgrade suggests the tests will fail.

@dataflake
Copy link
Member Author

Just to make sure this is not a macOS issue I tested this in a Linux sandbox as well. Same issue.

@tseaver
Copy link
Member

tseaver commented Aug 30, 2024

@dataflake All those super() calls do smell funky to me, but I don't really have a good grasp on how the pure-Python bit work there.

@tseaver
Copy link
Member

tseaver commented Aug 30, 2024

Also, I think one could make a case that running pyupgrade with in-place fixes isn't really a "linting" operation: it would likely be better for it to emit warnings that a human can check and fix, perhaps with an easy way to run the fixers.

@dataflake
Copy link
Member Author

dataflake commented Aug 30, 2024

I agree. I don't like things that change code as part of a test/lint run or as part of a commit. I originally pushed back, but there were more voices in favor of doing it this way.

@icemac
Copy link
Member

icemac commented Sep 4, 2024

What pyupgrade does is an automated refactoring to use newer, more modern language constructs. Usually I'd trust them. And our automated tests prevented us here from committing something bad.
I think we have way to much code for the little people maintaining it to promote more manual work.

@dataflake
Copy link
Member Author

A linting step running as part of the tests (be it tox or GHA) should only show the changes that are needed. They should not apply them automatically. Applying them should be a step initiated by the developer. So there could be a separate tox environment for that or a way to signal to the lint environment to apply the changes, maybe a command line argument or an environment variable. Do you think that is too much work for the developer? I think that's perfectly fine.

@icemac
Copy link
Member

icemac commented Sep 4, 2024

I personally see this as too much work because it creates friction. I do not like the programs just telling me: There is something wrong. It is easily fixable but I will not do it for you.
This always leaves me a bit sad, because I have to remember: How do I call the fixer. And then I have to wait until it has done its job. I always think: this could have been automated and done in the first run.

If you do not like to run on the linter/fixer your machine and later on commit the changes unter your name: There is no need to run it locally. It runs for each PR automatically and fixes things in the name of the machine.

@dataflake
Copy link
Member Author

I just have a different way of working: I do not want those automated fixes unless I can look at what it does beforehand. So just letting GHA do it doesn't make it any better for me. I want my code changes to be fixed and working before I push to GitHub, and I want full control over those changes. I don't want automated commits or pushes or automation changing my code unless I have a chance to vet the change and give my OK - all before any commit or push happens. That's why I added the --no-commit flag to the meta configuration script.

By the way, zopefoundation/zope.interface#322 cost me a lot of time because of this forced automation. A lot of tools kept making changes simultaneously and I had to do detective work to find out which tool broke the code and then try to fix it. Matter of fact, it took several runs because some tools made changes that other tools would change again. And the PR as a whole is hard to review because so many tools made changes. That's why I ran each manually and made separate commits.

I'd be happy if there was some way to signal to the linting/pre-commit step that I do not want it to change the code, just show me what it would do. So the default mode could remain the same, "write changes to disk". I could set some variable, either as command line argument or environment variable, to prevent writing to disk.

@tseaver
Copy link
Member

tseaver commented Sep 4, 2024

@icemac Not all fixers are equal: some are actually unsafe, making semantic changes to code.

@icemac
Copy link
Member

icemac commented Sep 5, 2024

Okay, you convinced me: We already seem to do too much for repos which did not see pre-commit yet. Let's move this discussion to zopefoundation/meta#277 instead of discussing on a merged PR where we will not be able to find the discussion after some time has passed.

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.

3 participants