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

Appply some refurb suggestions #759

Closed
wants to merge 6 commits into from

Conversation

DimitriPapadopoulos
Copy link
Contributor

No description provided.

[FURB108]: Replace `x == y or x == z` with `x in (y, z)`
[FURB109]: Replace `in [x, y, z]` with `in (x, y, z)`
[FURB110]: Replace `x if x else y` with `x or y`
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the refurb branch 3 times, most recently from 9fe2d99 to dc8c723 Compare December 31, 2023 15:02
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review December 31, 2023 15:05
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the refurb branch 3 times, most recently from fe44d0a to 2b98f34 Compare December 31, 2023 15:28
[FURB142]: Replace `for x in y: s.add(...)` with `s.update(... for x in y)`
[FURB179]: Replace `itertools.chain(*x)` with `itertools.chain.from_iterable(x)`
[FURB180]: Replace `metaclass=abc.ABCMeta` with `abc.ABC`
@brettcannon
Copy link
Member

Thanks for the PR, but it feels a bit noisy, e.g. changing lists to tuples that don't need to be changed. I'm going to close this, but if you think there's something I'm missing then please let us know or open a more targeted PR.

@brettcannon brettcannon closed this Jan 4, 2024
Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos left a comment

Choose a reason for hiding this comment

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

@brettcannon I can understand for changing lists to tuples, perhaps this one should be left out. The rest of the suggestions look like good ideas. The idea was to silence more ruff issue before adding a couple rules to jaraco/skeleton, for consistency across pypa projects. flake8-bugbear (B) rules are arguably more interesting, the tool has found two errors in the test code.

@@ -38,7 +38,7 @@ def __init__(self, requirement_string: str) -> None:

self.name: str = parsed.name
self.url: Optional[str] = parsed.url or None
self.extras: Set[str] = set(parsed.extras if parsed.extras else [])
self.extras: Set[str] = set(parsed.extras or [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should at least keep this one.

@@ -235,8 +235,7 @@ def test_specifiers_hash(self, specifier):

@pytest.mark.parametrize(
("left", "right", "op"),
itertools.chain(
*
itertools.chain.from_iterable(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following series of chain.from_iterable looks interesting to me as well.

@brettcannon
Copy link
Member

flake8-bugbear (B) rules are arguably more interesting, the tool has found two errors in the test code.

Then please open another PR w/ just those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants