Introduce multilineTrailingCommaBehavior configuration
#1044
Conversation
TTOzzi
requested review from
allevato,
bnbarham,
hamishknight and
rintaro
as code owners
TTOzzi
force-pushed
the
trailingCommasInLists
branch
from
f712fd2 to
c5fb5d2
Compare
TTOzzi
force-pushed
the
trailingCommasInLists
branch
from
c5fb5d2 to
a3de557
Compare
I was struggling because the point where a comma needs to be inserted differs for each syntax, but as I came to know about |
| case .ignore: | ||
| isCollection ? configuration.multiElementCollectionTrailingCommas : nil | ||
| } | ||
| if let shouldHandleCommaDelimitedRegion, shouldHandleCommaDelimitedRegion { |
There was a problem hiding this comment.
This could just be if shouldHandleCommaDelimitedRegion == true. Being written twice made me have to reread it a couple times to make sure they were referring to the same thing.
| protocol CommaSeparatedListSyntaxProtocol: SyntaxCollection where Element: WithTrailingCommaSyntax & Equatable { | ||
| /// Indicates whether the list should be treated as a collection for formatting. | ||
| /// If `true`, the list is affected by the `multiElementCollectionTrailingCommas` configuration. | ||
| var isCollection: Bool { get } |
There was a problem hiding this comment.
Hate to be nitpicky, but can you change this to isCollectionLiteral? isCollection feels like it could be talking about the node itself (which are all Collections).
Or... does this have to be a protocol requirement at all? It looks like you could just pass it in as another parameter to markCommaDelimitedRegion, since it has to be called individually for each node type anyway. That might be cleaner (but the rename is still valid elsewhere that it's referenced in the pretty printer).
There was a problem hiding this comment.
Yes, you're right. It was for a cleaner implementation.
I’ll make the update along with the rename!
| /// for collections only. In all other cases, existing trailing commas are preserved as-is and not modified. | ||
| /// If set to `.always` or `.never`, that behavior is applied uniformly across all list types, | ||
| /// regardless of `multiElementCollectionTrailingCommas`. | ||
| public var trailingCommasInMultilineLists: TrailingCommasInMultilineLists |
There was a problem hiding this comment.
Looking at this more closely, this feels too similar to the name of the other property below. I don't know if multilineLists really communicates the bit about parameter lists, tuples, closure captures, etc.
What about multilineTrailingCommaBehavior? Then, let's rename the enum to MultilineTrailingCommaBehavior and have the cases be alwaysUsed, neverUsed, and keptAsWritten. I think that helps to better document what's going on.
Later on (not in this PR), we could add a fourth case: onlyCollectionLiterals, which could take over the behavior of the other Boolean setting, and deprecate that one.
There was a problem hiding this comment.
Makes sense to me! I’ll update the rule name and case names as you suggested.
I have a question regarding the future direction.
From my personal perspective, I thought it might be better to keep the rule simple by handling multiline trailing comma without distinguishing collection literals, and deprecating the existing rule multiElementCollectionTrailingCommas.
Are you considering defining a case like onlyCollectionLiterals to preserve the behavior that distinguishes collection literals?
There was a problem hiding this comment.
I have a question regarding the future direction. From my personal perspective, I thought it might be better to keep the rule simple by handling multiline trailing comma without distinguishing collection literals, and deprecating the existing rule
multiElementCollectionTrailingCommas. Are you considering defining a case likeonlyCollectionLiteralsto preserve the behavior that distinguishes collection literals?
The key word there is "preserve". We have to have that option in order to preserve the existing behavior. Minimally, that means that we always support the existing Boolean for configurations written today, and if we need to maintain it long term, it would be nice to fold it into the new enum as well so there's just a single place to define this behavior.
There was a problem hiding this comment.
I understand. I’ll make sure to be careful when adding or changing rules. Thanks for explaining! 🙇
TTOzzi
force-pushed
the
trailingCommasInLists
branch
from
a3de557 to
89d887a
Compare
TTOzzi
changed the title
trailingCommasInMultilineLists configurationmultilineTrailingCommaBehavior configuration
Resolve #946
Added a configuration to support the use of trailing commas extended by SE-0439.
This configuration supports three values:
alwaysUsed: always insert trailing commas in multiline listsneverUsed: always remove trailing commaskeptAsWritten: preserve existing commas as-isThe default value is
keptAsWritten.The main concern was how to integrate this with the existing
multiElementCollectionTrailingCommasconfiguration.For now,
multilineTrailingCommaBehaviortakes higher precedence, but to minimize its impact onmultiElementCollectionTrailingCommas, it is implemented so that when set to the default valuekeptAsWritten, the behavior ofmultiElementCollectionTrailingCommasis preserved.I’d appreciate any suggestions or feedback you may have.