Skip to content

Fix bare except clause in setup.py - #5051

Open
koteshyelamati wants to merge 1 commit into
secdev:masterfrom
koteshyelamati:master
Open

Fix bare except clause in setup.py#5051
koteshyelamati wants to merge 1 commit into
secdev:masterfrom
koteshyelamati:master

Conversation

@koteshyelamati

Copy link
Copy Markdown

Replace bare except: with except ImportError: in setup.py.

The guarded block imports from setuptools — specifically from setuptools import .... The only exception that can reasonably be raised here is ImportError (when setuptools is not installed). Using except ImportError: makes the intent explicit and avoids accidentally swallowing KeyboardInterrupt or SystemExit.

@polybassa

Copy link
Copy Markdown
Contributor

Thanks for the PR, please fix the commit validity check.

@koteshyelamati

Copy link
Copy Markdown
Author

Thank you for the review, @polybassa! Could you clarify what the commit validity check requires? I see the "Check the validity of the commits" CI check is failing — is this a DCO sign-off (Signed-off-by) requirement, or a commit message format issue? I'll be happy to fix it once I know what's needed.

@polybassa

Copy link
Copy Markdown
Contributor
ERROR: Commit 00fd2e3ba5a9e3649e4116969a6838c3d1de10b5 is missing the 'AI-Assisted: yes|no [tool(s)]' trailer.

@itzzdev09 itzzdev09 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Confirmed correct, and I dug into why this one survived — which seems worth recording, since the fix alone doesn't stop it recurring.

E722 is not in the project's ignore list (ignore = E203, E731, W504, W503), so flake8 does flag bare excepts. Running it directly on the file:

$ flake8 setup.py
setup.py:19:1: E722 do not use bare 'except'
setup.py:97:89: E501 line too long (93 > 88 characters)

The reason CI never saw it is the lint target:

[testenv:flake8]
commands = flake8 scapy/

setup.py sits at the repo root, outside scapy/, so it's never linted. That also explains why this is the only bare except: left in the tree — grep finds exactly one occurrence repo-wide, and it happens to be in the one Python file the linter doesn't cover.

After the change in this PR, flake8 setup.py reports only the pre-existing E501 on line 97.

On correctness: from setuptools import ... raises ImportError when setuptools is missing (ModuleNotFoundError subclasses it, so that's covered too), and the narrowing does the intended job of no longer converting a KeyboardInterrupt during import into "setuptools is required to install scapy !".

Two things maintainers might want to decide alongside this:

  1. Whether to widen the lint target to catch regressions — something like flake8 scapy/ setup.py. That would also surface the E501 on line 97, so it isn't a zero-diff change.
  2. Whether the raise ImportError(...) should be raise ImportError(...) from None. As written the original traceback is chained in as __context__, so the user sees both the underlying import failure and the friendly message. That's arguably useful here, so it may be intentional — just noting it since the PR is already touching this handler.

Neither blocks the change; the PR is a strict improvement as-is.


Disclosure: reviewed with AI assistance (Claude Code). The flake8 output above is from actually running it against this file, before and after the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants