radeklat / delfino

A toolbox of command line helper script, wrapping tools used during Python development.
MIT License
12 stars 3 forks source link

Add option/config to the typecheck command #28

Closed shimpeko closed 2 years ago

shimpeko commented 2 years ago

Would like to add the followings for the typecheck command.

With this change, we will be able to specify mode per path.

For example we can run mypy for /src with strict and for /test without strict.

See https://github.com/radeklat/delfino/pull/28#issuecomment-1245531786 for the use case.

codecov[bot] commented 2 years ago

Codecov Report

Base: 48.19% // Head: 47.86% // Decreases project coverage by -0.32% :warning:

Coverage data is based on head (64ca30f) compared to base (2d94048). Patch coverage: 38.09% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #28 +/- ## ========================================== - Coverage 48.19% 47.86% -0.33% ========================================== Files 20 21 +1 Lines 720 750 +30 Branches 102 109 +7 ========================================== + Hits 347 359 +12 - Misses 370 388 +18 Partials 3 3 ``` | Flag | Coverage Δ | | |---|---|---| | integration_tests | `6.13% <7.14%> (+0.16%)` | :arrow_up: | | total | `47.73% <38.09%> (-0.33%)` | :arrow_down: | | unit_tests | `47.73% <38.09%> (-0.33%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/radeklat/delfino/pull/28?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t) | Coverage Δ | | |---|---|---| | [src/delfino/commands/typecheck.py](https://codecov.io/gh/radeklat/delfino/pull/28/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t#diff-c3JjL2RlbGZpbm8vY29tbWFuZHMvdHlwZWNoZWNrLnB5) | `36.95% <29.72%> (-10.67%)` | :arrow_down: | | [src/delfino/click\_utils/filepaths.py](https://codecov.io/gh/radeklat/delfino/pull/28/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t#diff-c3JjL2RlbGZpbm8vY2xpY2tfdXRpbHMvZmlsZXBhdGhzLnB5) | `100.00% <100.00%> (ø)` | | | [src/delfino/models/pyproject\_toml.py](https://codecov.io/gh/radeklat/delfino/pull/28/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t#diff-c3JjL2RlbGZpbm8vbW9kZWxzL3B5cHJvamVjdF90b21sLnB5) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Radek+L%C3%A1t)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jacksmith15 commented 2 years ago

Could this be configured via pyproject.toml rather than as arguments? I think one of the big advantages of delfino is that users don't need to know the arguments etc. they need to pass.

My thinking is that if we end up proxying lots of mypy options, then users might as well just use mypy directly. There are exceptions to this, e.g. the --maxfail argument on test is useful for TDD.

Interested in @radeklat 's thoughts

radeklat commented 2 years ago

I asked Shimpei about the motivation for this on Slack:

I’m using it here https://github.com/cookpad/global-python-utils/pull/124/files#diff-e0c3db65bec6563fb1667c02e1eb87c07363e9aa59e7eeb6c6dbba526beabfeaR32.

I agree with Jack on exposing this as a config option instead. As a list of one or more folders that should be checked strictly. It will make it more reusable too because no custom command will be needed. The implementation in global-python-utils already does that, only has the folders hardcoded.

Something like:

[tool.delfino.mypy]
strict_directories = [
    "src",
]
shimpeko commented 2 years ago

Thank you for your comments. I'll implement typecheck.strict_path configuration.


Could this be configured via pyproject.toml rather than as arguments? I think one of the big advantages of delfino is that users don't need to know the arguments etc. they need to pass. My thinking is that if we end up proxying lots of mypy options, then users might as well just use mypy directly. There are exceptions to this, e.g. the --maxfail argument on test is useful for TDD.

I think it is fine to have optional arguments with default values as the command can be run without knowing those options. Optional arguments are more flexible than pyproject.toml as we can configure them with python codes in sub/child commands.

In my case, I want to use python code to decide which path (can be given by --path option) to apply the --strict option, like;

    strict_path = delfino.sources_directory
    target_paths = [delfino.sources_directory, delfino.tests_directory, COMMANDS_DIRECTORY]
    if path:
        target_paths = [Path(p) for p in path]

    grouped_paths = groupby(target_paths, lambda current_path: strict_path in current_path.parents or current_path == strict_path)

    for force_typing, group in grouped_paths:
        click_context.invoke(typecheck, path=[str(g) for g in group], strict=force_typing)

For me, delfino is another invoke but more useful because;

  1. It provides commands with preset useful (almost de fact) options
  2. Users can tweak commands provided by delfino to meet their own requirements by using command nesting feature
  3. Users can share their own commands in multiple repositories using plugin system (not implemented yet)

And to maximise flexibility for option 2, I think it is better to make things configurable through command options (w/ default value) rather than pyproject.toml unless those options are project wides (shared by multiple commands). I also think it maybe reasonable to make command options configurable through both pytproject.toml and commands arguments.

I agree with the second point about proxy. My idea is to configure options with sub/child commands so that we don't need to pass options when executing commands.

I agree with Jack on exposing this as a config option instead. As a list of one or more folders that should be checked strictly. It will make it more reusable too because no custom command will be needed. The implementation in global-python-utils already does that, only has the folders hardcoded.

I think it is reasonable to add something like mypy.strict_path to pyproject.toml. It adds new capability to the typecheck commands.