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

Adding Pycln QA tool to the pre-commit config #5234

Merged
merged 5 commits into from
May 17, 2022

Conversation

hadialqattan
Copy link
Contributor

@hadialqattan hadialqattan commented Feb 25, 2022

Pull Request Check List

Resolves: #5233

  • Added tests for changed code. (no need)
  • Updated documentation for changed code. (no need)

Why?

I found a comment from one of your maintainers/active contributors that say a PR adding Pycln to the pre-commit hooks is welcome.

This is the comment:

A second PR adding pycln to the pre-commit config would be welcome, I think.

Consensus seems to be that the new Union syntax is harmless and nicer to write. Let's go ahead with this.

Originally posted by @neersighted in #5088 (comment)

Also, I found Pycln a good addition to improve the code quality of Poetry, therefore I've created this PR.

BTW, I'm the author of Pycln.

@@ -1,7 +1,6 @@
#!/usr/bin/env python
# Learn more: https://github.com/kennethreitz/setup.py
import os
import re
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detected and removed by pycln.

@@ -1,6 +1,6 @@
from distutils.core import setup

from build import *
from build import * # nopycln: import
Copy link
Contributor Author

Choose a reason for hiding this comment

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

# nopycln: import can be replaced with # noqa if you prefer that.

@@ -1,5 +1,4 @@
#!/usr/bin/env python
import io
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detected and removed by pycln.

@@ -1,2 +1,5 @@
from poetry.repositories.pool import Pool
from poetry.repositories.repository import Repository


__all__ = ["Pool", "Repository"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@@ -1 +1,4 @@
from poetry.puzzle.solver import Solver


__all__ = ["Solver"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@@ -1 +1,4 @@
from poetry.publishing.publisher import Publisher


__all__ = ["Publisher"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

from poetry.packages.package_collection import PackageCollection


__all__ = ["DependencyPackage", "Locker"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@@ -1,3 +1,6 @@
from poetry.mixology.solutions.solutions.python_requirement_solution import (
PythonRequirementSolution,
)


__all__ = ["PythonRequirementSolution"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@@ -1,3 +1,6 @@
from poetry.mixology.solutions.providers.python_requirement_solution_provider import (
PythonRequirementSolutionProvider,
)


__all__ = ["PythonRequirementSolutionProvider"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@@ -1 +1,4 @@
from poetry.masonry.builders.editable import EditableBuilder


__all__ = ["EditableBuilder"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@@ -1 +1,4 @@
from poetry.installation.installer import Installer


__all__ = ["Installer"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

Copy link
Member

@branchvincent branchvincent left a comment

Choose a reason for hiding this comment

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

thanks @hadialqattan! I'm a huge fan of this addition, and pycln. thanks for creating it 👏

I just have one question about the import you manually removed

.pre-commit-config.yaml Outdated Show resolved Hide resolved
src/poetry/packages/__init__.py Outdated Show resolved Hide resolved
src/poetry/utils/_compat.py Outdated Show resolved Hide resolved
@hadialqattan
Copy link
Contributor Author

thanks @hadialqattan! I'm a huge fan of this addition, and pycln. thanks for creating it 👏

I just have one question about the import you manually removed

Thank you for showing your interest in my tool, I appreciate that.

The import statement that I've removed manually seems like it's not used anywhere in the project.

@@ -3,3 +3,6 @@
from poetry.packages.dependency_package import DependencyPackage
from poetry.packages.locker import Locker
from poetry.packages.package_collection import PackageCollection


__all__ = ["DependencyPackage", "Locker", "PackageCollection"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why have I added __all__ dunder? __init__.py without __all__ (Pycln Docs)

@hadialqattan
Copy link
Contributor Author

I don't know why pre-commit is complaining about something has nothing todo with this PR:

pyupgrade................................................................Failed
- hook id: pyupgrade
- exit code: 1
- files were modified by this hook

Rewriting tests/console/commands/test_run.py
Rewriting tests/console/commands/test_show.py

@finswimmer
Copy link
Member

I don't know why pre-commit is complaining about something has nothing todo with this PR

Rebasing onto master should fix it now (see #5247)

branchvincent
branchvincent previously approved these changes Feb 28, 2022
Copy link
Member

@branchvincent branchvincent left a comment

Choose a reason for hiding this comment

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

thanks @hadialqattan, this looks good to me. let's wait for more opinions

@hadialqattan
Copy link
Contributor Author

hadialqattan commented Mar 4, 2022

BTW, I have included a used by section in the README file containing a list of notable projects/organizations that use Pycln.

@hadialqattan
Copy link
Contributor Author

hadialqattan commented Mar 10, 2022

@branchvincent

A gentle ping...

@neersighted neersighted reopened this May 4, 2022
@Secrus
Copy link
Member

Secrus commented May 17, 2022

@hadialqattan Could you please merge changes from master? I will make sure the team takes a look at this ;)

@Secrus Secrus added status/waiting-on-response Waiting on response from author Cleanup labels May 17, 2022
@hadialqattan
Copy link
Contributor Author

Hi @Secrus, sure, I'm gonna work on that as soon as possible. I'll let know you when the PR is ready to go.

@Secrus Secrus removed the status/waiting-on-response Waiting on response from author label May 17, 2022
@Secrus Secrus requested a review from a team May 17, 2022 16:34
@hadialqattan
Copy link
Contributor Author

hadialqattan commented May 17, 2022

@Secrus I believe that the PR is ready now.

@neersighted neersighted merged commit d0aa6e8 into python-poetry:master May 17, 2022
@branchvincent
Copy link
Member

@hadialqattan sorry for forgetting about this! If you're up for it, https://github.com/python-poetry/poetry-core would definitely benefit from this as well

@hadialqattan
Copy link
Contributor Author

@hadialqattan sorry for forgetting about this! If you're up for it, https://github.com/python-poetry/poetry-core would definitely benefit from this as well

That sounds good..

I'm gonna open a PR there as soon as possible!

@hadialqattan
Copy link
Contributor Author

@branchvincent it's ready PR360.

@kasteph kasteph mentioned this pull request May 30, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding pycln to the pre-commit config.
5 participants