Skip to content

Conversation

@paulinek13
Copy link
Contributor

Description

This PR is related to #653 and partially resolves #1176

  • added Ruff as a dev dependency
  • configured the ruff linter to check for docstring-related issues
  • fixed some of those issues inside /pyrit
  • added ruff-check pre-commit hook to block future PRs from introducing docstring issues that have been fixed in this PR

@romanlutz romanlutz changed the title DOC: introduce ruff-check for docstring quality enforcement MAINT: introduce ruff-check for docstring quality enforcement Nov 12, 2025
@romanlutz
Copy link
Contributor

@paulinek13 I love this! A few questions:

  • Do you think it's preferable to merge as is and then keep chipping away at the other issues over time? Holding up a PR like that can be painful in itself because things constantly change requiring more fixes.
  • Does it succeed right now?
  • Does it autofix issues or is it manual?

@paulinek13
Copy link
Contributor Author

paulinek13 commented Nov 12, 2025

@romanlutz thanks for the questions!

  • Do you think it's preferable to merge as is and then keep chipping away at the other issues over time? Holding up a PR like that can be painful in itself because things constantly change requiring more fixes.

Sorry I forgot to link to my comment that describes what's the plan here: #1176 (comment) 😄

In short, this PR only changes some issues (easy-to-fix issues) and adds them to ruff-check so that future PRs will be scanned for those. Mainly whitespace stuff.

Subsequent PRs will address the remaining issues related to the completeness of the public API, each focusing on one (or more) specific pyrit submodule (e.g., pyrit/auth or others). The key point is that these PRs will also need to adjust the ruff check so that the "fixed" modules are scanned for ALL issues going forward.

And then, when the last module is done, we'll just change the ruff precommit hook to scan whole /pyrit for all docstring issues.

  • Does it succeed right now?

✅ It does because the ruff hook currently only checks for issues that have been fixed in this PR


  • Does it autofix issues or is it manual?

Some issues could be fixed automatically. Example:
image
The last two could be safely and automatically fixed with --fix flag since they are marked with [*]

However, the precommit ruff-check introduced in this PR isn't automatically fixing anything yet. I still need to test how it will work with other hooks, like black for example.

@romanlutz
Copy link
Contributor

I need to read up a little, but perhaps this allows us to get rid of some existing hooks? Between all the linters we have there is probably quite a bit of overlap?

@paulinek13
Copy link
Contributor Author

I've removed the pylint hook that was only checking for unused imports so that it's now covered by Ruff’s F401 rule (pylint check took ~15s, now it's less than 1s 🙂).

As for an overlap, none of the existing hooks are checking docstring conventions and Ruff is currently only enforcing D*/DOC* rules + unused-imports. So there's no duplication in that sense.

From what I've found, other hooks that could be replaced by Ruff in the future are flake8, isort, and possibly black, but I think that switching those over would take a bit more effort and testing to make sure everything behaves the same.

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

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

Wonderful! As far as I am concerned, this is ready to merge. Just want to understand a few occurrences better first.

@romanlutz romanlutz merged commit 6c8303f into Azure:main Nov 17, 2025
20 checks 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.

TASK: improve docstrings completeness and quality across PyRIT modules

2 participants