-
Notifications
You must be signed in to change notification settings - Fork 455
Format Python and Bash scripts using ruff and shfmt #2115
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
The - from mongoc_common import *
+ from mongoc_common import mongoc_common_setup Prior to changes in this PR, the To diagnose the cause, removed
CC @rcsanchez97 It seems the
|
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.
LGTM
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.
LGTM with comments about some vendored files
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 file should probably be excluded from formatting, as it's a vendored external library.
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 file should be excluded from formatting, or the formatting should be added to the upstream copy.
sys.path.append(os.path.normpath(os.path.join(this_path, '../'))) | ||
|
||
from mongoc_common import * | ||
from mongoc_common import * # noqa: E402, F403 |
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.
Could this be rewritten to instead import the variables that are actually relevant for the sphinx config, rather than star-importing everything?
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.
I considered it, but opted to leave it as-is, since it seems the intended purpose of mongoc_common
is to act like an #include
rather than an import
(contents "copied" into each conf.py
file).
Followup to mongodb/mongo-cxx-driver#1454 which applies the same configuration and tools for formatting Python and Bash scripts to the C Driver using
ruff
andshfmt
respectively.As described in mongodb/mongo-cxx-driver#1454, the script formatter dependencies are defined in a separate
format-scripts
dependency group fromclang-format
to avoid impacting existing EVG lint tasks. Format configuration options are the same as for the C++ Driver (i.e. 2-space indentation for Bash scripts, 120 column width for Python scripts, etc.).Despite the high file diff count, most changes are trivial whitespace or sorting/grouping of
import
statements. Notable exceptions include:`cmd`
->$(cmd)
in Bash and sh scripts.;;
in somecase
statements in Bash scripts.""
->''
for non-multiline strings in Python scripts.'''
->"""
for multiline strings in Python scripts.x = lambda ...
->def x(): ...
in some Python scripts.import
statements. Those which depend on precedingsys.path
manipulation are silenced instead with# noqa: E403
.except:
->except Exception:
.Addressing F403 warnings by replacingSilenced F403 and F405 warnings due to the*
->mongoc_common_setup
forfrom mongoc_common import *
.import *
pattern in Sphinxconf.py
files.There was also the following linter error triggered by the
ruff check --select I --fix
command used to sort-and-groupimport
statements:This seems to be due to the use of a Python 3.12 feature in
src/libbson/tests/validate-tests.py
, which is accurate to the inline script metadata Python requirement of>=3.12
, but is inconsistent with the pyproject's Python version requirement of>=3.10
. Ruff currently only understands the pyproject requirements; it does not yet support inline script metadata (see: astral-sh/ruff#10457). Therefore,validate-tests.py
is manually excluded from the regular command and formatted separately with the--target-version py312
flag instead. All otherruff check
warnings are addressed by this PR as a drive-by improvement.