feature: add support for pyproject.toml configuration (#74)

[weibullguy]

I submit for your consideration a potential solution to issue #74.

Merging this PR will:

• Add pyproject.toml to the CONFIG_FILES list.
• Add conditional in _get_options to parse the configuration file with the correct tool.
• Add tomli as an extras_require in case someone oddly enough doesn't want to use pyproject.toml.
• Add an example pyproject.toml for testing and a test to use it.
• Update the README install and configuration sections.

make check and make test pass

My use cases (from CLI, as Makefile target, and as pre-commit hook) work when using my fork.

Closes #74

[Cielquan]

Could you please update the PR to the current state of the code base.

The file rstcheck.py no longer exists. It is now rstcheck/init.py.

[weibullguy]

Could you please update the PR to the current state of the code base.

The file rstcheck.py no longer exists. It is now rstcheck/init.py.

It looks like you didn't port over any of the tests for config files when you moved from test.bash to test_rstcheck.py. Do you want me to add tests for pyproject.toml or are you not planning to test the config files?

[Cielquan]

I moved the tests into a different file: test.bash -> tests/test_as_cli_tool.py.

So the unit tests should go into test_rstcheck.py and the system tests should go into test_as_cli_tool.py. I think I will need to state that in the development section of the docs or more like adding it at all.

I am currently working on rewriting or more like "reformatting" the tests in test_rstcheck.py to match the AAA pattern and pytest style.

And yes. Please add tests for pyproject.toml.

[Cielquan]

I updated the old test suite.

I also added a development section to the README

[weibullguy]

@Cielquan I think I have everything updated. All the tests are passing but the following QA check fails when I run tox -e pre-commit and when the GH actions runs against my fork.

safety...................................................................Failed
- hook id: python-safety-dependencies-check
- exit code: 2

usage: safety_check [-h] [--full-report] [--short-report] [--ignore IGNORE]
                    files [files ...]
safety_check: error: Unsupported mix of pyproject.toml & requirements files found

If I run safety check --full-report --file pyproject.toml from the CLI, it tells me it checked 0 packages. Not sure that's the correct result, but it's not throwing the same error as the pre-commit hook, so that's something.

Reading the pre-commit-hooks-safety repo, I see it checks all files with requirements in the name. The only files with requirements in the name are under .mypy-cache, .tox, and .venv on my end. But those should also be created when the action runs. I confirmed by deleting all three directories and re-running tox -e pre-commit; same error. I see the GH action passes on this repo, so obviously something is out of whack on my end, I just can't figure it out. Any ideas are welcome.

[Cielquan]

The safety issue is strange at first.

The hook should not look for any requirements files because it is set it to look for pyproject.toml files only.

The error you are seeing triggers when this statements is false: https://github.com/Lucas-C/pre-commit-hooks-safety/blob/master/pre_commit_hooks/safety_check.py#L40
This is the case when either no / 2+ files are passed or the file passed is not a pyproject.toml file.

The issue is the new examples/with_configuration/pyproject.toml file which also gets picked up by the hook. Therefore we have 2 files -> error.

Easiest solution is to change the .pre-commit-config.yaml file like this:

...
  - repo: https://github.com/Lucas-C/pre-commit-hooks-safety
    rev: d0c2c5156e146e5030e6aafff1a0cb398875b4f2 # frozen: v1.2.4
    hooks:
      - id: python-safety-dependencies-check
        args: ["--full-report"]
        files: pyproject.toml
+        exclude: examples
...

Will now take a look at the rest of the PR.

[Cielquan]

Thought I could propose changes with the file editing in the review. Seems not to be like this.

Anyways. For the toml extra to work I added tomli to the extras section.

The remaining review comments are below.

[Cielquan]

The report option can also be set in a config file. Please add it to the list.

[weibullguy]

I didn't include report in this originally because that option can only take one value. The ignore options can take more than one and, thus, require a list in pyproject.toml. It's only these list options that cause problems and need to be converted to a comma separated string. I changed the conditional to allow any of the options to be included in pyproject.toml as a string or a list of strings. You'll see I also changed the ignore_language option in the example pyproject.toml from ["cpp"] to "cpp" to cover this case.

I also wonder if recursive and/or debug should be added as well to completely cover all rstcheck options? Again, this would be scope creep for this PR, so I can raise an issue if you agree to the idea and want to tackle it later. Optionally, #98 could be revised to include the full scope of options you'd like the config files to support and I can make the necessary changes under that issue as well as updating the README to discuss the options.

[Cielquan]

report is a config value that is currently supported by the other config files and I think that is good. Therefore I would like pyproject.toml to do the same.

The config file is loaded for each file in their respective directory. This way you can set the report level in a nested config file without affecting files up the file tree. IMHO this is a valid reason. A corner case but valid.

recursive makes only sense if you give a directory. Therefore it IMO makes no sense to put into a config file because you specify no files/directories here.

debug is a special option you use actively and therefore it IMO makes also no sense to be put into a config file.

Further discussions please in a separate issue.

[weibullguy]

I think I wasn't clear in my earlier comment. I included report support in pyproject.toml from the beginning. But, because it can only be one value (info, warning, error, severe, none), there is no need for it to be a list in pyproject.toml. The ignore options can take multiple values which requires a list in pyproject.toml. The for loop, as I originally had it, was only converting the options that must be lists for multiple values in pyproject.toml to comma-separated strings.

This is what is returned when reading pyproject.toml:
{'ignore_directives': ['foobar', 'my-directive'], 'ignore_roles': ['some-custom-thing'], 'ignore_messages': ['(Document or section may not begin with a transition\\.$)'], 'ignore_language': ['cpp'], 'report': 'warning'}

This is the dict created from dict(configparser.items("rstcheck")):
{'ignore_directives': '\nfoobar,\nmy-directive,', 'ignore_roles': 'some-custom-thing', 'ignore_messages': '(Document or section may not begin with a transition\\.$)', 'ignore_language': 'cpp', 'report': 'warning'}

Since the remainder of the rstcheck functions are expecting the configparser format, the for loop is making the pyproject.toml output compatible (i.e., ['foobar', 'my-directive'] becomes 'foobar,my-directive,').

When adding report to the list of options to convert, it becomes necessary to put report = ["warning"] in pyproject.toml otherwise the for loop changes it to "w,a,r,n,i,n,g,". By having the conditional check if the option is a list and only convert it if so, it allows the user the option of report = ["warning"] or report = "warning" in pyproject.toml. This also means any of the config options can be either a list or a string which I think is less constraining on the user. It also more closely follows the format of .rstcheck.cfg in that single value options are simply report=warning.

See also my comment below.

[Cielquan]

I really don't like the idea of using an empty string instead of a None to signal that nothing was found.
And I do not see any hindrance to keep using None (see other comments).

[Cielquan]

Early returns are a good practice especially if you have an error or nothing to do. If we have no config file we have nothing to do and can return early. Why run the other if statements too? This only increases runtime. Therefore the original early return was very good.

Here is what I would do with this function

def _get_options(directory_or_file: str, debug: bool = False) -> typing.Dict[str, str]:
    config_path = find_config(directory_or_file, debug=debug)
    if not config_path:
        return {}

    if pathlib.Path(config_path).name == "pyproject.toml":
        return _get_pyproject_options(config_path)

    parser = configparser.ConfigParser()
    parser.read(config_path)
    try:
        return dict(parser.items("rstcheck"))
    except configparser.NoSectionError:
        return {}

Here we have early returns, which makes the code more readable IMO and shorter too. The nesting also decreases, which also improves readability.

If you use pathlib.Path or os.path does not make a big difference. I personally think pathlib.Path is more readable and I like it's API and handling more. Personal preference.

[Cielquan]

My version also improves the original try/except block with the early return there.

[Cielquan]

I do not like the idea to use comma-separated-string even so we have a list type in toml at hand.
When ignore_language is a string, why are the other ignore_* not?

[Cielquan]

This is a must change for me.

[weibullguy]

I only made this change to the example pyproject.toml to test both list and string for the ignore_* options.

See also my comment above, but, comma-separated-strings are allowable in pyproject.toml unless specifically prohibited by the tool from what I've seen. For example, black does the following to constrain the target-version option to a list:

target_version = config.get("target_version")
if target_version is not None and not isinstance(target_version, list):
    raise click.BadOptionUsage(
        "target-version", "Config key target-version must be a list"
    )

Without code to constrain pyproject.toml options to lists, the savvy user might use comma-separated-strings. But the documentation for pyproject.toml implies lists are required. There is no discussion in the documentation about using comma-separated-strings as far as I know. So I think common practice would be either lists, simple strings, or integers.

I can add code to constrain options to lists, but that's going to take me longer especially this time of year (spring is planting season). But, it's starting to feel to me like all the config file code needs to be restructured (like different classes for different config file formats) which would seem to fit with your plan to split things up after v6.0.0 is released.

Anyway, I'm not trying to be difficult, I've just been conditioned that way after years in the nuclear and aerospace industries ;). Just let me know how you want to proceed.

[Cielquan]

Mypy cannot control this, but the type hint is missing the List[str] doesn't it? Because the ignore_* configs are actually lists.

But see other comment before changing anything.

[Cielquan]

What do you think of this version for this function?

def _get_pyproject_options(config_path: str) -> typing.Dict[str, str]:
    RSTCHECK_TOML_CONFIG = typing.Dict[str, typing.Union[str, typing.List[str]]]

    with open(config_path, "rb") as conf_file:
        config = tomli.load(conf_file)
    
    options_from_file: typing.Optional[RSTCHECK_TOML_CONFIG] = config.get("tool", {}).get("rstcheck", None)

    if options_from_file is None:
        return {}

    options = {}

    # tomli returns a list of strings and ConfigParser returns a comma
    # separated string.  This makes the options from pyproject.toml consistent
    # with the options read from other configuration files.  The try block
    # accounts for pyproject files without a rstcheck section.
    for option in [
        "ignore_directives",
        "ignore_roles",
        "ignore_messages",
        "ignore_language",
        "report",
    ]:
        option_value = options_from_file.get(option, None)
        if option_value is None:
            continue
        
        if isinstance(option_value, list):
            option_value = ",".join(option_value)
        
        options[option] = option_value

    return options

Again early returns. I think the readability is good here with less nesting. Also the change of the try/except block to actually check the condition instead of silencing the error makes the intention more clear. This would also be another improvement one could make with my proposal for the try/except block in _get_options above.

[Cielquan]

Here I would also like to have the early return.
The rest is optional.

[Cielquan]

I appreciate your contribution and I do not think that you are difficult. I hope I am not too difficult myself. I think discussions are good and lead to better ideas and code.

I like typing very much and want to be correct with it. Yes, there is the option in toml to use comma-separated-strings and lists. But IMHO lists are the only plausible option here from the typing perspective and I think most people would think that you need to specify multiple items in a list by default. Therefore I would rather forbid comma-strings in toml by the application, like you suggested.

I think with the restructuring a lot of code will be changed dramatically anyways. Because of this and because I want to honor your time and effort I quickly made the adjustments, mentioned in my review above, myself and will merge them. Then I will start with the restructuring.

I hope you will forgive me this rather rude invasion into your PR. And I hope to have you on board later when the new topics, which arose here, are ready to be implemented. If you have the time and want to review my restructuring, I would appreciate it too.