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

chore: use fstrings #4794

Merged
merged 1 commit into from
Nov 28, 2021
Merged

Conversation

branchvincent
Copy link
Member

Pull Request Check List

Towards #4776

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Side note: would love to see commands.debug.info and commands.env.info refactored to use Cleo's Table instead of the ad-hoc alignment they have going on right now (as future work, not in this PR).

src/poetry/console/application.py Outdated Show resolved Hide resolved
src/poetry/console/commands/install.py Outdated Show resolved Hide resolved
src/poetry/mixology/term.py Outdated Show resolved Hide resolved
src/poetry/repositories/pypi_repository.py Show resolved Hide resolved
src/poetry/repositories/pypi_repository.py Show resolved Hide resolved
src/poetry/utils/password_manager.py Outdated Show resolved Hide resolved
src/poetry/utils/setup_reader.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
src/poetry/console/commands/config.py Outdated Show resolved Hide resolved
src/poetry/console/commands/install.py Outdated Show resolved Hide resolved
src/poetry/console/commands/install.py Outdated Show resolved Hide resolved
src/poetry/console/commands/install.py Outdated Show resolved Hide resolved
.flake8 Show resolved Hide resolved
@@ -818,8 +812,8 @@ def test_create_venv_uses_patch_version_to_detect_compatibility(
del os.environ["VIRTUAL_ENV"]

version = Version.parse(".".join(str(c) for c in sys.version_info[:3]))
poetry.package.python_versions = "^{}".format(
".".join(str(c) for c in sys.version_info[:3])
poetry.package.python_versions = "^" + ".".join(
Copy link
Member

Choose a reason for hiding this comment

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

Above note.

@@ -852,8 +846,8 @@ def test_create_venv_uses_patch_version_to_detect_compatibility_with_executable(
del os.environ["VIRTUAL_ENV"]

version = Version.parse(".".join(str(c) for c in sys.version_info[:3]))
poetry.package.python_versions = "~{}".format(
".".join(str(c) for c in (version.major, version.minor - 1, 0))
poetry.package.python_versions = "~" + ".".join(
Copy link
Member

Choose a reason for hiding this comment

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

Above? This one is a little different but maybe still doable.

tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
@neersighted
Copy link
Member

@branchvincent Sorry about the blockbuster review! There's a lot of churn here and it's important to both make sure our changes are beneficial as well as address any low-hanging code-quality fruit we discover. Thanks for bearing with me!

@neersighted
Copy link
Member

Hey @branchvincent, just wanted to see if you were close to finishing this -- given the amount of churn, conflicts will build up quickly. If not I can easily pick up the torch, and we can also discuss merging without some of my proposed refactors if you're blocked on those.

@branchvincent
Copy link
Member Author

thanks for the ping @neersighted, i've rebased / pushed my latest changes. the only thing i had left to address was the version specifier helper (see my reply #4794 (comment)). i'm 👍 to merge this and address that in a follow up

@@ -256,7 +242,7 @@ def and_to_string(
if this_line is not None:
buffer.append(" " + str(this_line))
Copy link
Member

Choose a reason for hiding this comment

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

I think we might as well turn these in to f-strings (with !s) for consistency.

@@ -256,7 +242,7 @@ def and_to_string(
if this_line is not None:
buffer.append(" " + str(this_line))

buffer.append(f" and {str(other)}")
buffer.append(f" and {other!s}")

if other_line is not None:
buffer.append(" " + str(other_line))
Copy link
Member

Choose a reason for hiding this comment

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

Above.

@@ -502,7 +498,7 @@ def test_deactivate_activated(
del os.environ["VIRTUAL_ENV"]

venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent))
version = Version.parse(".".join(str(c) for c in sys.version_info[:3]))
version = Version.from_parts(*sys.version_info[:3])
Copy link
Member

Choose a reason for hiding this comment

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

Awesome change! This pattern is really common and I'd like to see us replace it more generally.

@sonarcloud
Copy link

sonarcloud bot commented Nov 28, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
8.2% 8.2% Duplication

@neersighted neersighted merged commit 5814e33 into python-poetry:master Nov 28, 2021
@branchvincent branchvincent deleted the fstrings branch November 28, 2021 15:36
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.

2 participants