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

Read options from config #1668

Merged
merged 10 commits into from
Sep 1, 2020
Merged

Read options from config #1668

merged 10 commits into from
Sep 1, 2020

Conversation

vsalvino
Copy link
Contributor

This adds the ability to read options from a config file, such as setup.cfg, similar to how most other python tools work.

  • Documented in README.
  • Added basic unit test.

@sebweb3r
Copy link
Contributor

sebweb3r commented Aug 31, 2020

Nice MR :-)

But, I would prefer .codespellrc or .codespell/config or something similar.

@vsalvino
Copy link
Contributor Author

I am of the opposite mind, I prefer when all the tools can use one setup.cfg file 🙂 However I added the --config option to allow user to specify their own file.

@vsalvino
Copy link
Contributor Author

OK - I also added a magic .codespellrc file for you.

@vsalvino
Copy link
Contributor Author

Ok - I think this is finally ready to merge! On a separate note, it would be really helpful to have a requirements file and a contributing guide, to show how to run all of the various checks locally (unit tests, flake, etc.). Most of these are buried in the various CIs.

This is a really fantastic simple tool... I have been looking for a good native python spell checker for quite some time. Good work!

@larsoner
Copy link
Member

larsoner commented Sep 1, 2020

However I added the --config option to allow user to specify their own file.

I would rather just support traversing a list -- it's more common with Python tools AFAIK. For example:

https://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations

To start, looking for .codespell then setup.cfg (with the right header) would be good.

If you don't feel like implementing this, I'd say just kill the --config option for now and only look for setup.cfg, and someone else in a follow-up PR can add other standard-ish filenames.

@peternewman peternewman linked an issue Sep 1, 2020 that may be closed by this pull request
@peternewman
Copy link
Collaborator

@tomasbw does this do what you want?

README.rst Show resolved Hide resolved
@vsalvino
Copy link
Contributor Author

vsalvino commented Sep 1, 2020

To further clarify the order of config parsing:

  • First parse from command line (to check for existence of --config option).
  • Second parse from setup.cfg if it exists. This will override any cli args.
  • Third parse from .codespellrc if it exists. This will override cli or setup.cfg.
  • Fourth parse from file specified by --config, this will override cli, setup.cfg, .codespellrc.
  • Fifth parse the command line again, to let it override any of the config file options.

@larsoner
Copy link
Member

larsoner commented Sep 1, 2020

To further clarify the order of config parsing:

I think there is a problem with this hierarchy, I think command-line-passed arguments should have the highest priority. So in your list, it should be last, not first.

Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few comments

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
options = parser.parse_args(cfg_args)

# Re-parse command line options to override config.
options = parser.parse_args(list(args), namespace=options)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than re-calling this, would it be better to do:

if config.has_section(...):
    ...
    options = parser.parse_args(cfg_args)
else:
    options = argparse.NameSpace()

options = parser.parse_args(list(args), namespace=options)

If it's equivalent, it seems simpler/cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only re-parses if config.has_section(). The args have to be parsed first (line 372) before checking the config, to see if there is a --config option passed. And they have to be re-parsed at the end in order to override the options from the config file (which was the requested behavior). Following your suggestion, you would be re-parsing it twice if there were no config files, which seems pointless.

It's your project though, feel free to re-implement it yourself if you have a better idea.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh right, forgot about the need to parse to see the --config value. This should be fine then!

@larsoner larsoner merged commit 9672234 into codespell-project:master Sep 1, 2020
@larsoner
Copy link
Member

larsoner commented Sep 1, 2020

Thanks @vsalvino

@peternewman
Copy link
Collaborator

I guess we should dog food? This bit of Travis might be interesting, we'll have to override our skip config:

    - codespell --check-filenames --skip="./.git/*,*.pyc,./codespell_lib/tests/test_basic.py,./codespell_lib/data/*,./example/code.c,./build/lib/codespell_lib/tests/test_basic.py,./build/lib/codespell_lib/data/*"
    # this file has an error
    - "! codespell codespell_lib/tests/test_basic.py"

roobre pushed a commit to newrelic/helm-charts that referenced this pull request Apr 16, 2021
* .github/workflows/bump_chart_version.yaml: remove trailing whitespace

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

* .codespellrc: initial commit

Adds configuration for running codespell locally to check for possible
typos. In the future this configuration file should be used by CI as
well, but this is currently not possible until this PR gets merged:
codespell-project/codespell#1668

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

* .github/workflows: run spell checking as part of CI process

To avoid adding obvious typos.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

* Fix various typos found by codespell and manually

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add codespell project configuration file
5 participants