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: ruff rewrite dicts #2183

Merged
merged 2 commits into from
Jan 31, 2023
Merged

chore: ruff rewrite dicts #2183

merged 2 commits into from
Jan 31, 2023

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Jan 31, 2023

The built-in syntax is preferable to passing keywords to the dict constructor, which wastes a function call. Besides being more consistent and recognisable, it is also faster:

Python 3.11.1 (main, Dec 23 2022, 09:39:26) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import timeit
>>> timeit.timeit("dict(a=1, b=2)")
0.09944732999429107
>>> timeit.timeit("{'a': 1, 'b': 2}")
0.07909351703710854

This removes the ignoring of "C408".

@agoose77 agoose77 self-requested a review January 31, 2023 15:11
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #2183 (ba0084f) into main (a91d96b) will increase coverage by 0.04%.
The diff coverage is n/a.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/highlevel.py 77.04% <ø> (ø)
src/awkward/operations/ak_all.py 79.16% <ø> (ø)
src/awkward/operations/ak_any.py 79.16% <ø> (ø)
src/awkward/operations/ak_argcartesian.py 80.00% <ø> (ø)
src/awkward/operations/ak_argcombinations.py 82.35% <ø> (ø)
src/awkward/operations/ak_argmax.py 61.11% <ø> (ø)
src/awkward/operations/ak_argmin.py 61.11% <ø> (ø)
src/awkward/operations/ak_argsort.py 76.19% <ø> (ø)
src/awkward/operations/ak_broadcast_arrays.py 100.00% <ø> (ø)
src/awkward/operations/ak_cartesian.py 90.62% <ø> (ø)
... and 90 more

@henryiii henryiii temporarily deployed to docs-preview January 31, 2023 15:16 — with GitHub Actions Inactive
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

The dict constructor was only ever used in the entry-point from operations to their implementation functions. (See where the diffs are!)

The choice of dict constructor over built-in syntax was so that the function argument names would show up in the same color as they are in the entry-point function and the implementation function (as Python identifiers, not as strings). So there was a logic to it.

But activating C408 means that other instances of dict constructors can't accidentally slip in, in the future, and there's value in that.

I'm okay with making this change.

@jpivarski jpivarski enabled auto-merge (squash) January 31, 2023 18:13
@jpivarski jpivarski temporarily deployed to docs-preview January 31, 2023 18:24 — with GitHub Actions Inactive
@jpivarski jpivarski merged commit ac5c958 into main Jan 31, 2023
@jpivarski jpivarski deleted the henryiii/chore/ruff4 branch January 31, 2023 18:31
@henryiii
Copy link
Member Author

We can deactivate it for a specific subset of files (assuming it can be matched on) if you'd prefer.

Actually, could _errors.OperationErrorContext take this as **kwargs instead? Then you remove the nesting, too.

@agoose77
Copy link
Collaborator

In this case, I think we'd sooner replace the need to setup the context manager entirely with a decorator. We just haven't gotten around to it yet :)

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.

3 participants