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

Don't use whitespaces around brackets in collection expressions #37640

Closed
xamir82 opened this issue Oct 19, 2023 · 6 comments · Fixed by #37656
Closed

Don't use whitespaces around brackets in collection expressions #37640

xamir82 opened this issue Oct 19, 2023 · 6 comments · Fixed by #37656

Comments

@xamir82
Copy link

xamir82 commented Oct 19, 2023

On this page, the collection expressions in the code snippets have whitespaces around the brackets. This is an abomination. This style is not common is any language that uses brackets for array/collection initializations (e.g. JS, Python, etc.) and adds unnecessary noise to the code. It's also not what nearly any of the code examples in proposal looked like. Most notably, it's also inconsistent with list patterns in C# itself, a sort of aesthetic symmetry between list patterns and collection expressions was always touted as one of the important things about this feature.

This is a mistake. Please do not use this style.

I could make a PR to fix this. Let me know if you're on board.

@dotnet-bot dotnet-bot added the ⌚ Not Triaged Not triaged label Oct 19, 2023
@gfoidl
Copy link
Member

gfoidl commented Oct 20, 2023

There's also the opposite side: dotnet/runtime#93125 (comment)

@xamir82
Copy link
Author

xamir82 commented Oct 20, 2023

@gfoidl I would say that's a very idiosyncratic preference.
As I pointed out in my original comment, "no space around brackets" is the universal style in nearly all other languages who have a similar syntactic construct — the most notable examples being JavaScript, Python, Dart, and Rust.

And again, even in C# itself, list patterns don't use spaces around brackets either; which collection expressions, as @CyrusNajmabadi mentioned in the PR you linked to, were "modeled after".
Spaces around the brackets would also look awkward when you have a single element in the expression.

The dotnet/runtime#93125 was also eventually merged with the "no space around brackets" style. So, the runtime codebase now uses the same style as well.

There is every reason to update the documentation to reflect all of this.

@gfoidl
Copy link
Member

gfoidl commented Oct 20, 2023

Just wanted to point to a different opinion (which reflects also my opinion).

JavaScript for instance doesn't have custom indexers, so that reference doesn't apply here if we consider the distinction to indexers as valid point.

Also one could argue that array initializers in C# of the form int[] arr = { 1, 2, 3 }; has spaces around, so collection expressions (which "init" something) should have also spaces around.
List patterns don't "init" something, rather use it -- a bit more similar to indexers, so no spaces around is fine.

So what's an abomination for you, might be quite helpful for others.

In the end the use within .NET should be consistent (in either way), and everyone can choose the prefered style for their own projects.

@xamir82
Copy link
Author

xamir82 commented Oct 20, 2023

JavaScript for instance doesn't have custom indexers, so that reference doesn't apply here if we consider the distinction to indexers as valid point.

I don't see how custom indexers as a language feature is relevant here, JavaScript has the same obj[...] notation that custom indexers in C# would yield on the usage side of things. So the reference does very much apply. Correct me if I've misunderstood you, but I don't think that's a valid point.

Also one could argue that array initializers in C# of the form int[] arr = { 1, 2, 3 }; has spaces around, so collection expressions (which "init" something) should have also spaces around.

That's a non-sequitur IMO. The collection expression syntax is a different syntax, and its primary advantage over the existing syntaxes (e.g. the array init one) is its conciseness. There's no reason to for collection expressions to take inspiration from the very syntax they were introduced to replace.

List patterns don't "init" something, rather use it -- a bit more similar to indexers, so no spaces around is fine.

The idea is that collection expressions are supposed to "mirror" list patterns to establish a syntactic symmetry — and in general pattern matching "extraction" syntaxes should mirror their corresponding "creation" syntaxes — this was alluded to by Mads Torgersen for example, in his recent talks on C# 12.0.

Of course. you're totally free to use spaces around brackets if you like that formatting, no one's trying to take that away from you, this issue is just about the "official" style; and again, also considering the fact that the dotnet/runtime codebase is now using the no space style as well, in addition to the numerous other reasons I and others provided, it makes sense to update the docs accordingly.

@CyrusNajmabadi
Copy link
Member

FYI, the position of the C# team here is that list patterns and collection expressions do not have spaces around the brackets.

Similarly, slice patterns and spread elements do have whitespace. So you would write out a list pattern like so:

if (x is [start, .. middle, end])

And you would create a collection like so:

x = [start, .. middle, end];

These are consistent, and are going to be what our tools use. It would be preferred if any usages of collections from dotnet projects follows this style.

@CyrusNajmabadi
Copy link
Member

I opened #37656 for this. Thanks for the report.

@ghost ghost added the in-pr This issue will be closed (fixed) by an active pull request. label Oct 20, 2023
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Oct 23, 2023
@ghost ghost removed the in-pr This issue will be closed (fixed) by an active pull request. label Oct 23, 2023
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 a pull request may close this issue.

4 participants