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

chore: add inline noqa suppression #5159

Merged
merged 12 commits into from
Mar 12, 2025

Conversation

johubertj
Copy link
Contributor

@johubertj johubertj commented Mar 3, 2025

Description of changes:

  • s2n currently shows numerous linting warnings (F401 and F811) for code that is intentionally written this way (e.g., for test fixtures or managed processes). This PR adds inline # noqa directives to suppress these warnings

  • Removed lgtm [py/unused-import] from multiple lines since noqa handles suppression (for both ruff and CodeQL)

Command to Auto-Generate Inline noqa

  • Ran ruff check /home/ubuntu/s2n-tls/tests/integrationv2/{file_name} --add-noqa on multiple files to automatically append # noqa annotations where necessary.

Errors We're Suppressing & Justification

F811 (Redefinition) for managed_process – Pytest automatically injects fixtures, but Ruff incorrectly flags the manual import as a redefinition. Since the fixture is dynamically provided and not actually redefined, this warning is a false positive.

F401 (Unused Import) for managed_process – Ruff fails to recognize managed_process as used because it is passed as a function parameter. Since pytest fixtures are injected dynamically, the import is necessary, and the warning is incorrect.

F821 (Undefined) & F841 (Unused Variable) for input_view – input_view is only instantiated when stdin is registered after detecting send_marker, but since stdin isn’t initially registered, Ruff incorrectly assumes it is undefined or unused. The variable is always assigned before use.

@github-actions github-actions bot added the s2n-core team label Mar 3, 2025
@johubertj johubertj requested a review from jmayclin March 4, 2025 00:27
@johubertj johubertj changed the title chore: add inline noqa suppression directives chore: add inline noqa suppression Mar 10, 2025
@johubertj johubertj marked this pull request as ready for review March 10, 2025 19:21
@johubertj johubertj requested a review from dougch March 10, 2025 19:21
Copy link
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

I think this PR was entirely automated in generation? Can you include instructions about what commands were used to generate it?

Let's also include a description of all the errors that we are ignoring, and why it is safe/reasonable to do so.

@johubertj johubertj requested a review from jmayclin March 10, 2025 23:29
Copy link
Contributor

@jmayclin jmayclin left a comment

Choose a reason for hiding this comment

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

Discussed offline about F821 & F841. I don't particularly love allowing these, but I'm not too concerned given that it's a specific exception and not a blanket exception.

Additionally, I would say that some funky variable ordering is one of the tricky parts of that IO function 😉

@johubertj johubertj enabled auto-merge March 11, 2025 20:45
@johubertj johubertj added this pull request to the merge queue Mar 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2025
@johubertj johubertj added this pull request to the merge queue Mar 12, 2025
Merged via the queue into aws:main with commit ffb6079 Mar 12, 2025
46 checks passed
@johubertj johubertj deleted the chore/add-noqa-suppressions branch March 12, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants