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

Unexpected TRY400 indicated in unchanged code #292

Closed
jaraco opened this issue Sep 2, 2024 · 9 comments · Fixed by #293
Closed

Unexpected TRY400 indicated in unchanged code #292

jaraco opened this issue Sep 2, 2024 · 9 comments · Fixed by #293

Comments

@jaraco
Copy link
Member

jaraco commented Sep 2, 2024

In the feature/compilers-module-redux branch, I'm attempting to refactor the code. When I do, a TRY400 error is indicated even though the code hasn't changed:

__________________________________________________________________ test session ___________________________________________________________________
distutils/compilers/C/msvc.py:163:9: TRY400 Use `logging.exception` instead of `logging.error`
    |
161 |         ).decode('utf-16le', errors='replace')
162 |     except subprocess.CalledProcessError as exc:
163 |         log.error(exc.output)
    |         ^^^^^^^^^^^^^^^^^^^^^ TRY400
164 |         raise DistutilsPlatformError(f"Error executing {exc.cmd}")
    |
    = help: Replace with `exception`

First off, that's a nuisance error. What if what I really wanted was to log just the error message? Ruff shouldn't be griping about legitimate usage of supported interfaces. If logging.error shouldn't ever be used, they should file that as a feature change to CPython to get that functionality deprecated.

Second, why is this error only emitted in the branch (bac29f1), when the same code succeeds just fine in main?

@DimitriPapadopoulos What do you make of this error?

@pypa pypa deleted a comment Sep 2, 2024
jaraco added a commit that referenced this issue Sep 2, 2024
…e RUF100 to prevent it from failing in this branch. Ref #292
@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Sep 3, 2024

My understanding of the purpose of TRY400 is to suggest:

try:
    i = 1 / 0
except ZeroDivisionError as exc:
    logging.exception('"my error message"')

instead of:

try:
    i = 1 / 0
except ZeroDivisionError as exc:
    logging.error(f'"my error message" {exc}')

The output is considered more informative:

ERROR:root:"my error message"
Traceback (most recent call last):
  File "/volatile/home/dp165978/toto.py", line 4, in <module>
    i = 1 / 0
ZeroDivisionError: division by zero

instead of:

ERROR:root:"my error message" division by zero

In any case logging.exception was designed to log exceptions. However, I must admit it doesn't fit all use cases, especially when you just want to log exc.output and not details of exc! I guess TRY400 needs to be disabled globally.

@DimitriPapadopoulos

This comment was marked as outdated.

@DimitriPapadopoulos

This comment was marked as outdated.

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Sep 3, 2024

I can reproduce the issue with branch feature/compilers-module-redux after removing # noqa: RUF100, TRY400. That's really peculiar!

Files distutils/_msvccompiler.py from main branch and distutils/compilers/C/msvc.py from feature/compilers-module-redux are almost identical. Yet, after removing # noqa: RUF100, TRY400, the latter emits a TRY400 while the former does not. I'll keep looking.

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Sep 3, 2024

The reason is hidden in this change at the top of the file, in imports:

- from ._log import log
+ from ..._log import log

In the former case, log does not seem to be properly identified as logging.getLogger(), thus TRY400 is not emitted. I do see TRY400 emitted if I change:

from ._log import log

into:

import logging
log = logging.getLogger()

That looks like a ruff bug, doesn't it?

@DimitriPapadopoulos

This comment was marked as outdated.

@DimitriPapadopoulos
Copy link
Contributor

The bug is triggered by the distutils/__init__.py file:

.
└── distutils
    ├── __init__.py
    ├── _log.py
    └── _msvccompiler.py

Even an empty __init__.py file silences TRY400:

$ ruff check --select=TRY400
All checks passed!
$ 

After removing the empty __init__.py file:

.
└── distutils
    ├── _log.py
    └── _msvccompiler.py

I do see the TRY400 error:

$ ruff check --select=TRY400
distutils/_msvccompiler.py:6:5: TRY400 Use `logging.exception` instead of `logging.error`
  |
4 |     i = 1 / 0
5 | except DivisionByZero as e:
6 |     log.error(f"raised {e}")
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
  |
  = help: Replace with `exception`

Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
$ 

@DimitriPapadopoulos
Copy link
Contributor

I created astral-sh/ruff#13225.

@jaraco
Copy link
Member Author

jaraco commented Sep 3, 2024

Thanks for the investigation!

clrpackages pushed a commit to clearlinux-pkgs/pypi-setuptools that referenced this issue Sep 10, 2024
…version 74.1.2

Jason R. Coombs (9):
      Use monkeypatch to monkeypatch.
      Disable TRY400 so it doesn't cause problems in other branches. Disable RUF100 to prevent it from failing in this branch. Ref pypa/distutils#292
      In sdist.prune_file_list, support build.build_base as a pathlib.Path.
      Add test capturing missed expectation.
      Enable the test
      Add news fragment.
      Skip test on stdlib distutils
      Bump version: 74.1.1 → 74.1.2
      Removed test_integration tests (for easy_install).
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 a pull request may close this issue.

2 participants