Skip to content
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

Add codespell support (config, workflow to detect/not fix) and make it fix few typos #449

Merged
merged 7 commits into from
Jul 25, 2024

Conversation

yarikoptic
Copy link
Contributor

More about codespell: https://github.com/codespell-project/codespell .

I personally introduced it to dozens if not hundreds of projects already and so far only positive feedback.

CI workflow has 'permissions' set only to 'read' so also should be safe.

@@ -128,7 +128,7 @@ class Emitter:
indicator: Any,
need_whitespace: Any,
whitespace: bool = ...,
indention: bool = ...,
indentation: bool = ...,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

generated file, so might need to get to the "source" and skip this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

This file represents 3rd party code, so nothing I can do for this. Anything in typings should actually be ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, added typings to be skipped, redid codespell, forcepushed!

@pvandyken
Copy link
Contributor

Thanks, this should be fine. I'll take a look when I get a chance

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w -i 3 -C 2",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@pvandyken pvandyken added the maintenance Updates or improvements that do not change functionality of the code label Jul 17, 2024
Copy link
Contributor

@pvandyken pvandyken left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much for setting this up

@pvandyken
Copy link
Contributor

Looks like there's some linting errors (line length). Seems to be in docstrings, so I don't think ruff will fix automatically, they'll need to be reshaped with a line wrap tool.

@yarikoptic
Copy link
Contributor Author

Looks like there's some linting errors (line length). Seems to be in docstrings, so I don't think ruff will fix automatically, they'll need to be reshaped with a line wrap tool.

not poetry or ruff user, so your contribution to this PR would be most appreciated ;)

❯ poetry run ruff format snakebids
Skipping virtualenv creation, as specified in config file.
warning: `ruff format` is a work-in-progress, subject to change at any time, and intended only for experimentation.
ruff failed
  Cause: TOML parse error at line 215, column 12
    |
215 | [tool.ruff.lint]
    |            ^^^^
unknown field `lint`, expected one of `allowed-confusables`, `builtins`, `cache-dir`, `dummy-variable-rgx`, `exclude`, `extend`, `extend-exclude`, `extend-include`, `extend-ignore`, `extend-select`, `extend-fixable`, `extend-unfixable`, `external`, `fix`, `fix-only`, `fixable`, `output-format`, `force-exclude`, `ignore`, `ignore-init-module-imports`, `include`, `line-length`, `tab-size`, `logger-objects`, `required-version`, `respect-gitignore`, `select`, `show-source`, `show-fixes`, `src`, `namespace-packages`, `target-version`, `preview`, `task-tags`, `typing-modules`, `unfixable`, `flake8-annotations`, `flake8-bandit`, `flake8-bugbear`, `flake8-builtins`, `flake8-comprehensions`, `flake8-copyright`, `flake8-errmsg`, `flake8-quotes`, `flake8-self`, `flake8-tidy-imports`, `flake8-type-checking`, `flake8-gettext`, `flake8-implicit-str-concat`, `flake8-import-conventions`, `flake8-pytest-style`, `flake8-unused-arguments`, `isort`, `mccabe`, `pep8-naming`, `pycodestyle`, `pydocstyle`, `pyflakes`, `pylint`, `pyupgrade`, `format`, `per-file-ignores`, `extend-per-file-ignores`

or give me a hand here how to tame the poetry etc.

@pvandyken
Copy link
Contributor

not poetry or ruff user, so your contribution to this PR would be most appreciated ;)

Of course! Didn't want to touch your branch without your permission, but I'm happy to finish it off from here!

@yarikoptic
Copy link
Contributor Author

FWIW for above -- I have ancient ruff system wide... installed recent one in venv, but ruff check snakebids scripts --fix just checks and does not fix, expected? (I see you pushed, thanks! just want to learn howto for future)

@pvandyken
Copy link
Contributor

just checks and does not fix, expected? (I see you pushed, thanks! just want to learn howto for future)

Yeah, ruff won't touch docstrings or comments, so I just use a random line re-wrapping extension in my IDE.

@pvandyken
Copy link
Contributor

pvandyken commented Jul 23, 2024

Also the checks will probably fail because of some issue that came up in the nightly. Still have to look into this. But I know this PR doesn't change any code, so I'll just merge either way

UPDATE: I have the fix for the above ready to go, so I'll just wait for that PR

@pvandyken pvandyken merged commit 96fb5e1 into khanlab:main Jul 25, 2024
34 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Updates or improvements that do not change functionality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants