Skip to content

Remove parenthesis in getitem calls#4844

Merged
neutrinoceros merged 2 commits intoyt-project:mainfrom
cphyc:remove-parenthesis-in-getitem-calls
Mar 18, 2024
Merged

Remove parenthesis in getitem calls#4844
neutrinoceros merged 2 commits intoyt-project:mainfrom
cphyc:remove-parenthesis-in-getitem-calls

Conversation

@cphyc
Copy link
Member

@cphyc cphyc commented Mar 5, 2024

PR Summary

In accordance with the style of the code, replace all occurences of data[("gas", "density")] with data["gas", "density"] (no extra parentheses).

This is merely a stylistic change.

@cphyc cphyc force-pushed the remove-parenthesis-in-getitem-calls branch from 5e4ef52 to 5f1d904 Compare March 5, 2024 13:07
@neutrinoceros
Copy link
Member

I also prefer that style but I recall that there wasn't a consensus last time it was discussed.
However, I also think we shouldn't bikeshed over this.
Since the patch is here now but may quickly grow some conflicts, I would recommend merging soon, but not so soon that other maintainers wouldn't get a chance to voice objections. So, if it turns out to be uncontroversial after all, I'd merge this in about 2 weeks.

@matthewturk
Copy link
Member

I feel like once upon a time we did it this way, and then when we moved to mandating field tuples we changed it, and I don't think I particularly cared for the change then but I honestly don't know that I am that invested one way or the other now. It does feel like a lot of churn though.

@cphyc
Copy link
Member Author

cphyc commented Mar 5, 2024

To be fair I'm also happy for this not to be merged. However, at present, we're using both styles in the codebase (depending on who wrote it), so for the sake of uniformity, it'd be marginally better to settle on either one of the formats (data[("gas", "density")] vs data["gas", "density"]).

@cphyc cphyc added enhancement Making something better proposal Proposals for enhancements, changes, etc code style Related to linting tools labels Mar 5, 2024
@cphyc cphyc marked this pull request as ready for review March 5, 2024 13:27
@cphyc
Copy link
Member Author

cphyc commented Mar 5, 2024

@yt-fido test this please...

@chrishavlin
Copy link
Contributor

chrishavlin commented Mar 6, 2024

+1 from me for removing parenthesis. also agree it'd be nice to merge ASAP if folks agree with the change.

@matthewturk
Copy link
Member

bandaids are meant to be ripped off, I suppose!

@matthewturk
Copy link
Member

And actually, how hard would it be to make a rule for ruff to apply this? Probably easy to write, hard to confine to just yt data objects, now that I'm thinking of it.

@neutrinoceros
Copy link
Member

I had the same idea but I'm too intimidated to try it out myself :D

hard to confine to just yt data objects, now that I'm thinking of it.

I believe parentheses are never required anywhere that looks like [(... ,...)], because they are syntactically meaningless there, so I think this would actually work as a general purpose listing rule !

@matthewturk
Copy link
Member

I believe parentheses are never required anywhere that looks like [(... ,...)], because they are syntactically meaningless there, so I think this would actually work as a general purpose listing rule !

Yeah, you must be right. I think I got confused because of the way h5py recommends [()] which is specifically an empty tuple, so I think I got a bit mixed up.

@neutrinoceros
Copy link
Member

Would you like to give it a go or is it okay if we just open a feature request on ruff ?

@cphyc
Copy link
Member Author

cphyc commented Mar 15, 2024

Id say let's open an issue; I won't have time to implement this anytime soon.

@neutrinoceros
Copy link
Member

My question was meant for Matt, sorry for being unclear !

@neutrinoceros
Copy link
Member

No conflict after two weeks and no objection in sight, so I'll push the button. Thanks all !

@neutrinoceros neutrinoceros enabled auto-merge March 18, 2024 15:15
@neutrinoceros neutrinoceros merged commit 192f26b into yt-project:main Mar 18, 2024
@cphyc cphyc deleted the remove-parenthesis-in-getitem-calls branch March 18, 2024 15:20
@neutrinoceros
Copy link
Member

Btw I just opened a feature request on ruff astral-sh/ruff#11990

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

Labels

code style Related to linting tools enhancement Making something better proposal Proposals for enhancements, changes, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants