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

chore: apply codespell pre-commit hook #460

Closed
wants to merge 2 commits into from

Conversation

huxuan
Copy link
Member

@huxuan huxuan commented Apr 8, 2024

No description provided.

@huxuan huxuan linked an issue Apr 8, 2024 that may be closed by this pull request
@huxuan huxuan requested a review from msclock April 8, 2024 01:10
@huxuan huxuan force-pushed the xuan.hu/codespell branch from 69e673b to c969b26 Compare April 8, 2024 01:47
@msclock
Copy link
Contributor

msclock commented Apr 8, 2024

why don't enable autofix on codespell.🤔 but it's all ok.

@huxuan
Copy link
Member Author

huxuan commented Apr 8, 2024

why don't enable autofix on codespell.🤔 but it's all ok.

What do you mean by enabling autofix? I did not find any reference for that.

@msclock
Copy link
Contributor

msclock commented Apr 8, 2024

why don't enable autofix on codespell.🤔 but it's all ok.

What do you mean by enabling autofix? I did not find any reference for that.

codespell can autofix some common typo by using flags --check-filename --write-changes.

  • --check-filename: check file names as well.
  • --write-changes: write back to spell typo in place if possible.
    So the autofix snippet will be:
  - repo: https://github.com/codespell-project/codespell
    rev: v2.2.6
    hooks:
      - id: codespell
        args:
          - --check-filenames
          - --write-changes

But it seems you adopt toml as additional dependencies, the flags may be placed in pyproject.toml.

@msclock
Copy link
Contributor

msclock commented Apr 8, 2024

Plus, codespell is a python cli, and it can be used as dev or lint dependencies for convenient local use.🤔

@huxuan huxuan marked this pull request as draft April 8, 2024 06:51
@huxuan
Copy link
Member Author

huxuan commented Apr 9, 2024

Hi @msclock, I started a discussion [1] on pdm project here about managing dev dependencies. I would like to known your preference especially for those standalone development tools, such as code-spell, toml-sort, ruff and etc. I can see several options as following:

  1. pre-commit, this is the suggested, but tends to encounter network issues as we have discussed.
  2. pipx may not be a good choice to install too many applications IMO since they are likely to have conflicts and likely to be overridden when working on different projects.
  3. tox and nox might be a choice since we can manage everything within the project.

Any comments?

BTW, why I have concern on this is trying to avoid explosion of lockfile, considering that we will have many other tools like check-jsonschema.

[1] pdm-project/pdm#2783

@msclock
Copy link
Contributor

msclock commented Apr 9, 2024

Hi @msclock, I started a discussion [1] on pdm project here about managing dev dependencies. I would like to known your preference especially for those standalone development tools, such as code-spell, toml-sort, ruff and etc. I can see several options as following:

  1. pre-commit, this is the suggested, but tends to encounter network issues as we have discussed.
  2. pipx may not be a good choice to install too many applications IMO since they are likely to have conflicts and likely to be overridden when working on different projects.
  3. tox and nox might be a choice since we can manage everything within the project.

Any comments?

BTW, why I have concern on this is trying to avoid explosion of lockfile, considering that we will have many other tools like check-jsonschema.

[1] pdm-project/pdm#2783

@huxuan the thought of deps categories seems quite good. Different tools adopt different ways. my initial ideas:

  • pipx/nox manage Pre-conmit.
  • pre-commit installs or just runs lint tools like ruff and the installation of tools can be delegated to pipe or nox.
  • pdm manages test and docs deps.

I consider we are just separating installation dependent of those independent tools. And its runnings can be designed to resolve other issues by nox/tox/pipe.

@huxuan
Copy link
Member Author

huxuan commented Apr 11, 2024

Hi @msclock

Just for double confirm, I summarized a list according to our project. Please take a look and double check we are on the same page.

  1. Standalone tools managed by pipx
    • copier
    • pdm
    • pre-commit
    • toml-sort
    • ruff
    • codespell
  2. Semi-dependent tools included in dev-dependencies but locked with a different lock file
    • mypy (needs installation of prod dependencies for type checking)
    • sphinx and the whole doc group (needs installation of the project to extract version)
  3. Fully dependent tools included in dev-dependencies and locked with the main pdm.lock
    • pytest and the whole test group

Actually, the only thing I am not sure is whether we should treat mypy same as sphinx or pytest.

For the tox/nox, I still have a little concern to involve more tools since it bring more complexity for template users, but since we will use pipx anyway, we can first keep using it. Moreover, as I have implemented the install-all command [1], it might be easy to users to install all of them with one-line command with the new version of pipx.

[1] pypa/pipx#1301

@huxuan
Copy link
Member Author

huxuan commented Apr 11, 2024

And I think there could be a rule for our pre-commit config, we should try to use the local pre-commit hooks especially for those python-based standalone tools. Other than the official pre-commit-hooks which are consist of several Python scripts, we should only use those third-party pre-commit directly ONLY when it is not Python based.

@msclock
Copy link
Contributor

msclock commented Apr 11, 2024

Hi @msclock

Just for double confirm, I summarized a list according to our project. Please take a look and double check we are on the same page.

  1. Standalone tools managed by pipx

    • copier
    • pdm
    • pre-commit
    • toml-sort
    • ruff
    • codespell
  2. Semi-dependent tools included in dev-dependencies but locked with a different lock file

    • mypy (needs installation of prod dependencies for type checking)
    • sphinx and the whole doc group (needs installation of the project to extract version)
  3. Fully dependent tools included in dev-dependencies and locked with the main pdm.lock

    • pytest and the whole test group

Actually, the only thing I am not sure is whether we should treat mypy same as sphinx or pytest.

For the tox/nox, I still have a little concern to involve more tools since it bring more complexity for template users, but since we will use pipx anyway, we can first keep using it. Moreover, as I have implemented the install-all command [1], it might be easy to users to install all of them with one-line command with the new version of pipx.

[1] pypa/pipx#1301

great job👍 keep the use of pipx is good choice. thus those things will not conflict again.

for nox/tox it is working as the same as the project makefile. I consider we could keep it for now.

mypy needs to inspect the project and its deps, so I consider it should be kept with devs-dependencies.

@msclock
Copy link
Contributor

msclock commented Apr 11, 2024

And I think there could be a rule for our pre-commit config, we should try to use the local pre-commit hooks especially for those python-based standalone tools. Other than the official pre-commit-hooks which are consist of several Python scripts, we should only use those third-party pre-commit directly ONLY when it is not Python based.

it is true practical for python projects, especially accounting for network issues.

non-python projects need to verify. because it requires a deps registry well managed to install them by pip or pipx.

@huxuan
Copy link
Member Author

huxuan commented Apr 11, 2024

Ok then I will close this pull request and create an issue to manage these dependencies well.

for nox/tox it is working as the same as the project makefile. I consider we could keep it for now.

Yep, I personally prefer Makefile since it is most likely to be natively available. Just want to mention that pdm also support scripts, maybe we can unified to use pdm only in the future.

non-python projects need to verify. because it requires a deps registry well managed to install them by pip or pipx.

Absolutely, I just assume we are talking about ss-python here. Thanks for the clarification anyway.

@huxuan huxuan closed this Apr 11, 2024
@huxuan huxuan mentioned this pull request Apr 11, 2024
7 tasks
@huxuan huxuan deleted the xuan.hu/codespell branch April 11, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integreate codespell check (with local pre-commit)
2 participants