ynput
Ruff settings
Problem
Current ruff settings are to prohibitive.
We can reverse the logic, select all rules and just ignore those that we must. Here is the list of rules I had to disable in 3dequalizer addon for some reasons I'll try to explain here:
- D203 - one-blank-line-before-class: exclusive with D211[^1]
- D213 - multi-line-summary-second-line, exclusive with D212[^1]
- D406 - new-line-after-section-name [^1]
- D407 - dashed-underline-after-section [^1]
-
PTH - this is ignoring all "use pathlib" warnings. We have a lot of
os.pathlogic all over the place and rewriting it all to make use ofpathlibwould be major work. -
ANN101 - this is deprecated check where
selfshould be type annotated for the sake of older IDE that couldn't infer the type. -
ANN102 - the same thing as ⬆️ but for
cls -
ANN204 - return type annotation for special methods. This requires even
__init__()to have that[^2] - COM812 - missing-trailing-comma - while this rule make sense, it is enforcing trailing commas even within function arguments if multiline.
- S603 - subprocess-without-shell-equals-true. Requires validation of input arguments, but it is vague how to achieve it?
- ERA001 - commented-out-code - code that is commented out is IMO still valuable as illustration or example.
- TRY003 - raise-vanilla-args - define new exception if message is too long. This just adds more code that needs to be maintained.
-
UP007 - non-pep604-annotation - since we need to support python 3.7, we can't use
|in type annotations. - ARG002 - unused-method-argument - for method that are overriden, even unused arguments should be listed for clarity, no?
-
INP001 - implicit-namespace-package - this rule is forcing
__init__.pyto be almost in all directories with any python files even in a case where nothing will ever be imported from there. for docstrings ("D" rules) we should addpydocstyle.convention = "google" -
UP006 - non-pep585-annotation - this is for compatibility with py37 -
List[int] = [1, 2, 3]vs.list[int] = [1, 2, 3] -
UP007 - non-pep604-annotation - this is for compatibility with py37 -
foo: Union[int, str] = 1vs.foo: int | str = 1 - UP035 - deprecated-import - py37 compatibility too, when you need to import something from deprecated location
[^1]: This should be solved by pydocstyle.convention = "google" so no need to ignore
[^2]: Unless mypy-init-return is used in ruff config.
Some notes:
- Most of the error are due to the docstrings - like missing package level docstring, etc. D100, D104, D107
- printf style formatting: there are some collisions with how we use printf style in logging. We shouldn't basically. We should make use
extraargument in logging:
# wrong
self.log.info(f"this is {error}")
# also wrong
self.log.info("this is %s" % error)
# also wrong
self.log.info("this is {}".format(error))
# this is right
self.log.info("this is %s, error)
printf style formatting: there are some collisions with how we use printf style in logging. We shouldn't basically. We should make use extra argument in logging:
Slows down pyblish plugins logging. Also when using dict/list/more complex objects, you have to explicitly convert it to string.
Slows down pyblish plugins logging.
How does it slow it down? Because it should speed it up because it'd skip formatting logs that remain unhandled.
I mean, technically it could be slower because we just happen to capture ALL logs.
Also when using dict/list/more complex objects, you have to explicitly convert it to string.
Regarding this - I believe that's actually an issue of how the 'logs' are handled by us - we store handle the unformatted string I suppose instead of the formatted one? Which means we format it later when the object has changed in the meantime? I've never seen this occur with regular logging at least - but may be just me.
I mean, technically it could be slower because we just happen to capture ALL logs.
We do, and pyblish does it too, and if it's logged to console, then it is formatted 3 times. Any additional handler is +1 formatting.
Regarding this - I believe that's actually an issue of how the 'logs' are handled by us - we store handle the unformatted string I suppose instead of the formatted one? Which means we format it later when the object has changed in the meantime? I've never seen this occur with regular logging at least - but may be just me.
It does not happen in pyblish because we're handling it at 2 places. We do format the message at the time of emiting instead of using, so we get what was logged at the time of emit.
Anyways, good start. We can discuss more, when we start to apply them to repositories.
How does it slow it down? Because it should speed it up because it'd skip formatting logs that remain unhandled.
printf style is slowing stuff down because it is always evaluated even in the case the log isn't emitted after all. But that is the case for all formatting except the last mentioned, the extra log() argument.
printf style is slowing stuff down because it is always evaluated even in the case the log isn't emitted after all.
In pyblish plugins are all logs emited, and are always formatted at the time of their emit. At least log handlers that are used by default (and it is not possible to avoid using them without rewriting pyblish). So, for pyblish logging it is actually faster to use f-string, because it is formatted only once. If we would use log formatting then it is formatted 1x + len(handlers) 100% (at least 2x in publisher tool).
If we would not do it then we would get invalid logs:
data = {}
self.log.info("%s", data)
data["key"] = "value"
self.log.info("%s", data)
>>> INFO: {'key': 'value'}
>>> INFO: {'key': 'value'}
In that case we can add exception for pyblish plugins. Something like this could work:
[tool.ruff.lint.per-file-ignores]
"*/plugins/publish/*.py" = ["G002"] # because of how logging is done in pyblish
so living and working with it for some time I found that it also needs handling of tests, because it is bitching about asserts etc. So I also added:
[tool.ruff.lint.per-file-ignores]
"client/ayon_core/lib/__init__.py" = ["E402"]
"tests/*.py" = ["S101", "PLR2004"]
