Unfriendly error in the absence of `tool.pysen.lint` section

sergeant-wizard commented 2 years ago

When pyproject.toml does not contain a tool.pysen.lint section, running pysen run lint results in the following error:

pysen run lint
usage: pysen run [-h] [--error-format {gnu}] [--no-parallel] {} [{} ...]
pysen run: error: argument targets: invalid choice: 'lint' (choose from )

In our case I believe the best configuration is no configuration at all, so I suggest enabling black, isort, flake and mypy without explicit configuration by default. By doing so we will not need to directly address the superficial issue of the unfriendly error.

bonprosoft commented 2 years ago

Sorry for my late response!!!!!

Let me clarify.

If tool.pysen is missing in pyproject.toml (or pysen.toml) at the directory or any ancestor directories, we will get the following error, right?

Could not find either a pyproject.toml file or a pysen.toml file containing a [tool.pysen] section in this or any of its parent directories. 
The `--loglevel debug option` may help.

The case you mentioned is that there exists a pyproject.toml that has a tool.pysen section, but doesn't have any tool.pysen.lint section. Is my understanding correct?

In our case I believe the best configuration is no configuration at all, so I suggest enabling black, isort, flake and mypy without explicit configuration by default.

Even if we enable those options by default, pysen should require the pyproject.toml to have one or more pysen options (like version) at least to enable pysen for the directory, right?

I know that some repositories using pysen adopt the monorepo style; they put pysen.toml at the repository root and the pyproject.toml for the packaging purpose at each package directory. I want to support such use cases.

...IMHO, just showing a hint/help message on that case can be helpful to everyone and a reasonable solution for us.

sergeant-wizard commented 2 years ago

Yes, the current behavior requires at least [tool.pysen] section to be present, but will there be any harm in making [tool.pysen.lint] optional (and by default enabling the 4 linters)?

bonprosoft commented 2 years ago

but will there be any harm in making [tool.pysen.lint] optional?

pros

Less configuration for most use cases :+1:

cons of the proposal

Becomes a breaking change (i.e. users may need to update their configurations)
Users may need to disable options if new linters/formatters are supported in the future version of pysen
- (* I don't think it happens unless something unusual happens :roll_eyes: )

So +1 to keep the current behavior. But I would like to support your opinion if you and @Hakuyume think the suggestion is reasonable for us :+1:

Hakuyume commented 2 years ago

Let me confirm the proposed behavior.

case A: there is no `pyproject.toml` .

pysen raises an error since pysen cannot detect the project root.

case B: there is a `pyproject.toml`. But it does not contain any pysen-related sections.

pysen raises an error since pysen cannot detect the project root.

[tool.foo]
bar = "baz"

case C: there is a `pyproject.toml`. It contains some pysen-related sections (e.g. `tool.pysen.version`) but does not contain `[tool.pysen.lint]`.

pysen uses a default configuration and does not raise any errors.

[tool.pysen]
version = "0.10"

case D: there is a `pyproject.toml`. It contains `[tool.pysen.lint]` but does not contain any `enable_***` fields in it.

pysen does noting since no linters are enabled.

[tool.pysen]
version = "0.10"

[tool.pysen.lint]
py_version = "py37"

bonprosoft commented 2 years ago

Thanks @Hakuyume !

case D: there is a pyproject.toml. It contains [tool.pysen.lint] but does not contain any enable_*** fields in it

Good point!

IMO, the linters should also be enabled by default in case D. That would be less confusing for me.

Also, we should rename enable_something to disable_something?

Hakuyume commented 2 years ago

IMO, the linters should also be enabled by default in case D.

If so, how about the following case (case E)?

case E: there is a `pyproject.toml`. It contains `[tool.pysen.lint]` and some linters are enabled explicitly.

[tool.pysen]
version = "0.10"

[tool.pysen.lint]
enable_black = true

bonprosoft commented 2 years ago

For case E, all the linters are still enabled; the same as the enable_black = false in the current configuration.

(Another possible behavior is to disable linters except black, like --enable and --disable options in pysen run CLI)

As it's confusing, we should rename the options to disable_something. The default value (or the zero-value) of a boolean in the config should be false.

Hakuyume commented 2 years ago

Thank you for your confirmation. I prefer keeping the current behavior (i.e. require [tool.pysen.lint] section) and updating the error message, since the behavior for case D and E sounds confusing to me.

pfnet / pysen