Chore: Fix typos and linter errors #196

antirotor · 2024-03-19T11:25:42Z

Changelog Description

Fixing typos and linter errors in the repository.

Additional info

Based on #192 this is using codespell and ruff to cleanup the repo. (Un)fortunately any typo-fixed is marking the file as a food for ruff and so it starts triggering linter errors in pre-commit hook preventing the commit. The only way around it is obey the ruff tool - and that is a good thing, isn't it?

Note

This is using #192 for tools

Fixing workflow

run codespell (you can set it as External tool in PyCharm for example) - ./.poetry/bin/poetry run codespell
take the list of errors, fix them and run ruff on that changed file - ./.poetry/bin/poetry run ruff check /path/to/file.py
fix linter errors
commit

MustafaJafar · 2024-03-25T07:12:07Z

Hey, I gave it a shot.
Here are my exact testing steps.

I'm not used to install poetry manually. So, please correct me if any of the following steps are wrong.

Copy tools\manage.ps1 from ayon-launcher repo
Run ./tools/manage.ps1 create-env
./.poetry/bin/poetry run codespell
./.poetry/bin/poetry run ruff check client\ayon_core\hosts\houdini\api\lib.py .. (cool, no errors to fix)
./.poetry/bin/poetry run ruff check client\ayon_core\hosts\houdini\api\pipeline.py .. (found 1 fixable error with --fix)
./.poetry/bin/poetry run ruff check client\ayon_core\hosts\houdini\api\pipeline.py --fix .. (cool, found and fixed 1 error)
git commit .. (cool, all clean.. here's my log)

PS E:\Ynput\ayon-core> git add client/ayon_core/hosts/houdini/api/pipeline.py
PS E:\Ynput\ayon-core> git commit -m "fix linting -  houdini pipeline.py"
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to C:\Users\Mustafa Taher\.cache\pre-commit\patch1711349959-35712.
[INFO] Initializing environment for https://github.com/codespell-project/codespell.
[INFO] Initializing environment for https://github.com/codespell-project/codespell:tomli.
[INFO] Initializing environment for https://github.com/astral-sh/ruff-pre-commit.
[INFO] Installing environment for https://github.com/codespell-project/codespell.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/astral-sh/ruff-pre-commit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
don't commit to branch...................................................Passed
codespell................................................................Passed
ruff.....................................................................Passed
[INFO] Restored changes from C:\Users\Mustafa Taher\.cache\pre-commit\patch1711349959-35712.
[chore/fix_typos e432149d4f] fix linting -  houdini pipeline.py
 1 file changed, 1 deletion(-)

So, is that the expected behavior ?
Do I need to push any commits? or Did the commit part from the testing steps ?

antirotor · 2024-03-25T11:06:26Z

So, is that the expected behavior ?
Do I need to push any commits? or Did the commit part from the testing steps ?

The stuff running in pre-commit hooks is run against the changed files to be commited - so if they are without typos and ruff isn't complaining, then you are fine. But try to fix typo it one of the huge files, like maya/api/lib.py - that will force you to ruff-clean the whole file :)

antirotor · 2024-03-26T13:49:52Z

There is a place for fine tuning:

if you need to add some words to be ignored by codespell (like parms used in Houdini), you can edit pyproject.toml and add it to the ignore-words-list under [tools.codespell].

Warning

words in ignore-words-list must be lower-case otherwise they will be ignored.

In similar manner, ruff can be tweaked per file - remember, there is no file-wide noqa directive possible. But you can get around it using this configuration option:

[tool.ruff.lint.per-file-ignores]
"__init__.py" = ["E402"]

this will apply E402 for all __init__.py files (import must be on the top of the file)

BigRoy · 2024-03-26T13:50:04Z

client/ayon_core/pipeline/farm/pyblish_functions.pyi

This file does not match the function in the .py file - shall we remove it completely? OR fix it up? e.g. remap_source does not have a source argument.

Also, it lacks many functions that are in the .py file.

we can remove it - this was there mainly for python 2 compatibility and with ayon-core, we can drop it.

BigRoy · 2024-03-26T13:51:00Z

server_addon/tvpaint/server/settings/publish_plugins.py

@@ -1,5 +1,5 @@
 from ayon_server.settings import BaseSettingsModel, SettingsField
-from ayon_server.types import ColorRGBA_uint8, ColorRGB_uint8
+from ayon_server.types import ColorRGBA_uint8


While we're at it. Should we also remove the commented lines on lines 16-18 along with this?

The lines are actually correct and should be used. But that would require to implement conversion of settings in server addon.

I wanted to do that when we split TVPaint to real addon.

BigRoy · 2024-03-26T13:53:21Z

client/ayon_core/hosts/aftereffects/api/__init__.py

@@ -9,7 +9,6 @@
 )

 from .pipeline import (
-    AfterEffectsHost,


I wonder if instead of removing this it should actually be added to the __all__ below instead? (Looking at what the api/__init__.py seems to do for e.g. Blender or Maya hosts.

iLLiCiTiT · 2024-03-26T13:55:01Z

client/ayon_core/hosts/hiero/api/startup/Python/Startup/otioexporter/OTIOExportTask.py

@@ -7,7 +7,6 @@
 import hiero.core
 from hiero.core import util

-import opentimelineio as otio


Isn't this here to make sure otio is imported?

BigRoy · 2024-03-26T13:56:06Z

client/ayon_core/hosts/nuke/api/plugin.py

-            # append reformated tag
-            add_tags.append("reformated")
+            # append reformatted tag
+            add_tags.append("reformatted")


I'm pretty sure we should not be changing these "tags" keys unless we're entirely sure that ALL other code relying on reformated to be refactored as well?

E.g. here:

ayon-core/client/ayon_core/plugins/publish/extract_review.py

Line 1228 in 464fc4c

reformat_in_baking = bool("reformated" in new_repre["tags"])

and the use of the word reformated occurs in many other places instead of just here.

iLLiCiTiT · 2024-03-26T13:58:45Z

client/ayon_core/hosts/tvpaint/plugins/create/create_render.py

@@ -766,8 +766,7 @@ def _rename_groups(
                group["red"],
                group["green"],
                group["blue"],
-                group_name
-            )
+                )


Suggested change

)

)

iLLiCiTiT · 2024-03-26T14:01:17Z

client/ayon_core/modules/loader_action.py

@@ -13,7 +13,6 @@ def tray_init(self):
        # Add library tool
        self._loader_imported = False
        try:
-            from ayon_core.tools.loader.ui import LoaderWindow


Purpose of the method is to make sure the tool can be imported, removing the import makes it useless.

iLLiCiTiT · 2024-03-26T14:02:59Z

client/ayon_core/scripts/slates/slate_base/api.py

@@ -1,15 +0,0 @@
-from .font_factory import FontFactory


I think removing content of api.py in this case is not a good idea. Probably rather add __all__.

iLLiCiTiT · 2024-03-26T14:04:35Z

server/settings/publish_plugins.py

@@ -720,7 +720,7 @@ class IntegrateHeroVersionModel(BaseSettingsModel):

 class CleanUpModel(BaseSettingsModel):
    _isGroup = True
-    paterns: list[str] = SettingsField(
+    patterns: list[str] = SettingsField(


This was kept as paterns on purpose for backwards compatibility. This would require to handle the change of the model in server addon.

iLLiCiTiT · 2024-03-26T14:06:27Z

client/ayon_core/__init__.py

@@ -1,5 +1,4 @@
 import os
-from .version import __version__


This is here on purpose...

iLLiCiTiT · 2024-03-26T14:10:25Z

client/ayon_core/hosts/unreal/lib.py

    commandlet_cmd = [f'{ue_editor_exe.as_posix()}',
                      f'{cmdlet_project.as_posix()}',
-                      f'-run=AyonGenerateProject',
+                      '-run=AyonGenerateProject',
                      f'{project_file.resolve().as_posix()}']


commandlet_cmd = [ ue_editor_exe.as_posix(), cmdlet_project.as_posix(), '-run=AyonGenerateProject', project_file.resolve().as_posix() ]

iLLiCiTiT · 2024-03-26T14:11:04Z

client/ayon_core/modules/base.py

@@ -1,13 +1,8 @@
 from ayon_core.addon import (
-    AYONAddon,


They are here on purpose ayon_core.modules exists only for backwards compatibility.

iLLiCiTiT · 2024-03-26T14:11:56Z

client/ayon_core/pipeline/create/product_name.py

@@ -1,4 +1,3 @@
-import ayon_api


Is it ok to have first line empty?

iLLiCiTiT · 2024-03-26T14:30:11Z

client/ayon_core/hosts/houdini/plugins/load/load_alembic.py

@@ -58,7 +58,7 @@ def load(self, context, name=None, namespace=None, data=None):

        normal_node.setInput(0, unpack)

-        null = container.createNode("null", node_name="OUT".format(name))
+        null = container.createNode("null", node_name="OUT".format())


Suggested change

null = container.createNode("null", node_name="OUT".format())

null = container.createNode("null", node_name="OUT")

iLLiCiTiT · 2024-03-26T14:30:23Z

client/ayon_core/hosts/houdini/plugins/publish/validate_cop_output_node.py

@@ -72,5 +72,5 @@ def get_invalid(cls, instance):
        if output_node.type().category().name() != "Cop2":
            raise PublishValidationError(
                ("Output node %s is not of category Cop2. "
-                 "This is a bug...").format(output_node.path()),
+                 "This is a bug...").format(),


Suggested change

"This is a bug...").format(),

"This is a bug..."),

iLLiCiTiT · 2024-03-26T14:31:26Z

client/ayon_core/hosts/maya/plugins/publish/extract_unreal_skeletalmesh_fbx.py

@@ -74,7 +74,7 @@ def process(self, instance):
            renamed_to_extract.append("|".join(node_path))

        with renamed(original_parent, parent_node):
-            self.log.debug("Extracting: {}".format(renamed_to_extract, path))
+            self.log.debug("Extracting: {}".format(renamed_to_extract, ))


Suggested change

self.log.debug("Extracting: {}".format(renamed_to_extract, ))

self.log.debug("Extracting: {}".format(renamed_to_extract))

iLLiCiTiT · 2024-03-26T14:39:57Z

This just can't be merged in single PR, or it would take too long. The PR contains crutial bugfixes, some changes that can be merged without much thinking, but also many questionable changes that need discussions. I'll start to make separate PRs based on changes here. We can keep this as reference.

MustafaJafar · 2024-04-01T08:57:25Z

I gave it another shot. I found out that /tools/manage.ps1 was added recently in another PR. (Cool!)

Setup my environment I only need to run ./tools/manage.ps1 create-env.
Running ./.poetry/bin/poetry run codespell listed a lot of files that code speed is not happy with.
Fixing a typo manually and making a commit worked. (the checks only ran onto my modified files only)

I think this PR works but I wonder why do I in some case find inconsistent results ? (I have code-spell from this PR and VS extension)

Running tool provided by this PR:
It looks cool

PS E:\Ynput\ayon-core> ./.poetry/bin/poetry run codespell client\ayon_core\hosts\houdini\plugins\create\create_staticmesh.py
PS E:\Ynput\ayon-core>

However, In the VS extension

A no0b question. (sorry I need to check my understanding).

we have ruff for linting.
we have code spell for Spelling checks.
both tools check same code but for two different things. and when they conflict, ruff's opinion is stronger.

Fixing workflow mentioned in the PR description is basically:

check against ruff
check against code-spell
fix errors
check again against ruff (to ensure that spell fixes don't conflict with linting)

So, was that summary correct ?

Another question:
should we create follow up PRs to fix spelling ?
As, code-spell checks the entire file not only the changes which can be irrelevant in some PRs.

moonyuet · 2024-04-24T08:26:19Z

Tested with this code and it works as expected. Just wondering if this can be also applied to the addon repos which are already separate(e.g. zbrush, resolve etc.)? Or I have to add it into client folder for the check in the current stage?

antirotor · 2024-05-13T13:02:07Z

closing as this was already merged via different PRs and is working

✏️ fyx tipos partz 1

17e3a70

antirotor added the type: enhancement Improvement of existing functionality or minor addition label Mar 19, 2024

antirotor requested review from jakubjezek001 and iLLiCiTiT March 19, 2024 11:25

antirotor assigned simonebarbieri, kalisp, MustafaJafar, antirotor, iLLiCiTiT and moonyuet Mar 19, 2024

ynbot added host: After Effects host: Houdini module: Deadline size/L labels Mar 19, 2024

Merge branch 'enhancement/add-ruff-and-code-spell' into chore/fix_typos

5223fda

iLLiCiTiT mentioned this pull request Mar 25, 2024

Enhancement: add ruff workflow and codespell pre-commit hooks #192

Merged

antirotor and others added 3 commits March 25, 2024 12:07

Merge branch 'develop' into chore/fix_typos

bdf7029

Merge branch 'develop' into chore/fix_typos

3de1fc7

🔧 additional fixes

303082a

BigRoy reviewed Mar 26, 2024

View reviewed changes