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.

Sign up for GitHub

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

Jump to bottom

Issue 3241: CLI tool to explain violations #3256

Merged

sobolevn merged 55 commits into wemake-services:master from Tapeline:issue-3241

Jan 28, 2025

Contributor

Tapeline commented Jan 5, 2025

I have made things!

Checklist

I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
I have created at least one test case for the changes I have made
I have updated the documentation for the changes I have made
I have added my changes to the CHANGELOG.md

Related issues

#3241 CLI tool to explain issues

🙏 Please, if you or your company is finding wemake-python-styleguide valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/wemake-python-styleguide. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.

Tapeline added 4 commits

January 6, 2025 00:19


          WIP wemake-services#3241

784f7de

Add explain command


          Tidy up so all checks could pass. Update CHANGELOG.md

7ad0c7a


          Update docs

7bff6aa


          Remove unused docutils

8c469bf

sobolevn reviewed

View reviewed changes

Member

sobolevn left a comment

Thanks! There's a lot of work ahead, this is not a full review :)
I will look again tomorrow.

docs/pages/usage/cli.rst Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/application.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/application.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/base.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/message_formatter.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/message_formatter.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/violation_loader.py Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/violation_loader.py Outdated Show resolved Hide resolved

Member

sobolevn commented Jan 5, 2025

Please, rebase your PR.

Tapeline changed the title ~~Issue 3241~~ Issue 3241: CLI tool to explain violations

Tapeline and others added 23 commits

January 6, 2025 13:10


          WIP wemake-services#3241

daf71cc

Add explain command


          Tidy up so all checks could pass. Update CHANGELOG.md

b359f0e


          Update docs

a13e022


          Remove unused docutils

bdd8f41


          Apply suggestions from code review

fd99365


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

4ee0aed

# Conflicts:
#	CHANGELOG.md
#	docs/pages/usage/cli.rst
#	wemake_python_styleguide/cli/application.py
#	wemake_python_styleguide/cli/commands/base.py
#	wemake_python_styleguide/cli/commands/explain/command.py
#	wemake_python_styleguide/cli/commands/explain/message_formatter.py
#	wemake_python_styleguide/cli/commands/explain/violation_loader.py


          Ruff fixes


          [pre-commit.ci] auto fixes from pre-commit.com hooks

b9074a1


          WIP wemake-services#3241

6365a2b

Add explain command (with code review and updated docs)


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

9334b0b

# Conflicts:
#	CHANGELOG.md
#	docs/pages/usage/cli.rst
#	poetry.lock
#	tests/test_cli/test_explain.py
#	wemake_python_styleguide/cli/application.py
#	wemake_python_styleguide/cli/cli_app.py
#	wemake_python_styleguide/cli/commands/explain/message_formatter.py
#	wemake_python_styleguide/cli/commands/explain/violation_loader.py
#	wemake_python_styleguide/cli/output.py


          Re-lock poetry

f9b983b


          [pre-commit.ci] auto fixes from pre-commit.com hooks

ff23e7b


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

3a3d56a


          Update tests accordingly. Fix docs

5060ace


          [pre-commit.ci] auto fixes from pre-commit.com hooks

4eca1fa


          Try fixing docs again. Add more tests

c167f0b


          [pre-commit.ci] auto fixes from pre-commit.com hooks

c85ae45


          More tests to cover almost all cli utility

c062e76


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

a065ad1


          [pre-commit.ci] auto fixes from pre-commit.com hooks

4533f63


          Cover 100% CLI

a8695cb


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

54405e1

# Conflicts:
#	tests/test_cli/test_explain.py


          [pre-commit.ci] auto fixes from pre-commit.com hooks

4ef4db1


          Remove unused pragma no cover from cli_app.py

88f7f82

Tapeline requested a review from sobolevn

January 24, 2025 13:25

Contributor Author

Tapeline commented Jan 24, 2025

Applied all changes except removing hasattr check (reasoning provided).
Thank you for patiently reviewing my code)

sobolevn reviewed

View reviewed changes

Member

sobolevn left a comment

Thank you! Looks like that this is the ~last review round :)

wemake_python_styleguide/cli/__init__.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/cli_app.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/__init__.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/__init__.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/command.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/message_formatter.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/violation_loader.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/violation_loader.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/output.py Show resolved Hide resolved

wemake_python_styleguide/cli/output.py Show resolved Hide resolved

Tapeline and others added 6 commits

January 25, 2025 16:50


          Delegate no argument corner-case to argparse. Move doc link to consta…

2978cd7

…nts (formatter.py also affected). Move from using hardcoded module names to module discovery with glob.


          [pre-commit.ci] auto fixes from pre-commit.com hooks

9ea66b7


          Update conditions so python 3.10 tests pass too

82100b2


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

ecf631a


          Update so python 3.10 tests pass too

8c68b6d


          Add pragma no cover to previous fix as it only runs on python 3.10

2a54118

Tapeline requested a review from sobolevn

January 25, 2025 12:57

sobolevn reviewed

View reviewed changes

Member

sobolevn left a comment

I saw some new things to improve 😊

tests/test_cli/test_explain.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain.py

+                      universal_newlines=True,
+                      encoding=encoding,
+                      env=os.environ,
+                      shell=True,

Member

sobolevn Jan 25, 2025

why do we need shell= and env=?

Member

sobolevn Jan 25, 2025

Marking as not really resolved. I think that we might need to use shell=True because wps is not locatable without the PATH, right?

tests/test_cli/test_explain.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain.py Show resolved Hide resolved

wemake_python_styleguide/cli/cli_app.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/message_formatter.py Outdated Show resolved Hide resolved

Tapeline and others added 2 commits

January 25, 2025 20:17


          Separate unit and integration tests. Delegate indentation removal to …

b03371e

…textwrap library


          [pre-commit.ci] auto fixes from pre-commit.com hooks

2969b6c

sobolevn reviewed

View reviewed changes

Member

sobolevn left a comment

We are almost there! This is going to be an amazing feature, thanks a lot to your for delivering it! 👍

tests/test_cli/test_explain.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain.py

+                      universal_newlines=True,
+                      encoding=encoding,
+                      env=os.environ,
+                      shell=True,

Member

sobolevn Jan 25, 2025

Marking as not really resolved. I think that we might need to use shell=True because wps is not locatable without the PATH, right?

tests/test_cli/test_explain_internals.py Outdated Show resolved Hide resolved

tests/test_cli/test_explain_internals.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/cli_app.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/cli_app.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/base.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/command.py Outdated Show resolved Hide resolved

Contributor Author

Tapeline commented Jan 25, 2025

Also thought about a slight improvement: we can add a shorthand for explain subcommand. It may be wps e, for example

Member

sobolevn commented Jan 25, 2025

I don't think that adding some shorthands is actually a good thing :)

Tapeline and others added 3 commits

January 27, 2025 01:35


          Separate unit and integration tests. Delegate indentation removal to …

…textwrap library. Add typed arguments for subcommands


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

52ed4ff

# Conflicts:
#	tests/test_cli/test_explain.py
#	tests/test_cli/test_explain_internals.py
#	wemake_python_styleguide/cli/cli_app.py
#	wemake_python_styleguide/cli/commands/explain/message_formatter.py


          [pre-commit.ci] auto fixes from pre-commit.com hooks

4a976ca

sobolevn reviewed

View reviewed changes

Member

sobolevn left a comment

Thanks!

wemake_python_styleguide/cli/cli_app.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/base.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/command.py Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/command.py Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/command.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/base.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/base.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/module_loader.py Outdated Show resolved Hide resolved

Tapeline added 4 commits

January 27, 2025 20:22


          Clean up redundant abstractions. Move to old generics syntax to suppo…

017e271

…rt python 3.10.


          Merge remote-tracking branch 'origin/issue-3241' into issue-3241

d502cc2

# Conflicts:
#	wemake_python_styleguide/cli/application.py
#	wemake_python_styleguide/cli/commands/base.py
#	wemake_python_styleguide/cli/commands/explain/command.py


          Remove application that came back after merging

56fc859


          Fix some things that broke after merge

7950dc9

Tapeline requested a review from sobolevn

January 27, 2025 17:52

sobolevn approved these changes

View reviewed changes

Member

sobolevn left a comment

Thank you!

wemake_python_styleguide/cli/commands/explain/module_loader.py Outdated

+              def get_violation_submodules() -> Collection[ModuleType]:
+                  """Get all possible violation submodules."""
+                  return _safely_get_all_submodules(_VIOLATION_MODULE_BASE)

Member

sobolevn Jan 28, 2025

Do we really need _safely_get_all_submodules? It is used in a single place, but all this function does is calling _safely_get_all_submodules

So, let's just remove _safely_get_all_submodules and move all code to get_violation_submodules

wemake_python_styleguide/cli/commands/explain/module_loader.py Outdated Show resolved Hide resolved

wemake_python_styleguide/cli/commands/explain/module_loader.py Outdated Show resolved Hide resolved


          Apply suggestions from code review

481212e

sobolevn merged commit dffbb62 into wemake-services:master

9 checks passed

Member

sobolevn commented Jan 28, 2025

You did a great job here, awesome feature! Thanks a lot for your patience during the review time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet