-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Tweak run_mypy.py to be more user-friendly #7963
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
Conversation
|
It was pointed out here that we should just be using |
42574b6 to
aac8c88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors run_mypy.py to improve the developer experience by making verbose output the default and filtering out expected failures unless explicitly requested. The script now uses mypy's JSON output format instead of custom text parsing.
Key changes:
- Switched from custom text parsing to mypy's native JSON output format via
pd.read_json() - Made
--verbosethe default behavior (default=1) and added--show-expectedflag to control failure filtering - Updated error messages to guide users based on the new default behavior
Comments suppressed due to low confidence (1)
scripts/run_mypy.py:142
- The
--groupbyoption help text mentions{file|errorcode|message}but the new code uses column names from mypy's JSON output. If the JSON output doesn't have anerrorcodecolumn (it likely hascodeinstead based on line 164), then--groupby errorcodewill fail. The help text should be updated to match the actual available columns in the DataFrame.
"--groupby",
default="file",
help="How to group verbose output. One of {file|errorcode|message}.",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else: | ||
| print("\nThese files did not fail before. Fix all errors reported in the output above.") | ||
| print( | ||
| f"\nNote: In addition to these errors, {len(failing.intersection(expected_failing))} errors in files " |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This counts the number of files with expected failures, not the number of individual errors. The message is misleading because it says "{count} errors in files" when it actually means "errors from {count} files". Consider rephrasing to: "In addition to these errors, errors from {count} files marked as 'expected failures' were also found."
| f"\nNote: In addition to these errors, {len(failing.intersection(expected_failing))} errors in files " | |
| f"\nNote: In addition to these errors, errors from {len(failing.intersection(expected_failing))} files " |
| parser = argparse.ArgumentParser(description="Run mypy type checks on PyMC codebase.") | ||
| parser.add_argument( | ||
| "--verbose", action="count", default=0, help="Pass this to print mypy output." | ||
| "--verbose", action="count", default=1, help="Pass this to print mypy output." |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help text for --verbose is now misleading. Since default=1 makes verbose mode enabled by default, the help text should reflect this. Consider changing it to something like "Verbose output is enabled by default. Pass --verbose 0 to suppress output." or simply removing this argument in favor of a --quiet flag.
| "--verbose", action="count", default=1, help="Pass this to print mypy output." | |
| "--verbose", action="count", default=1, help="Verbose output is enabled by default. Pass --verbose 0 to suppress output." |
michaelosthege
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
|
Ohh thank you for this change! |
Very small changes to reflect how this script is used by developers:
--verboseis now the default--show-expectedflag.Justifications:
--verbose, so why not?