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

Adjustments to split FunctionParameterSyntax into multiple nodes for function parameters, closure parameters and enum parameters #495

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 28, 2023

@ahoppen ahoppen requested a review from allevato March 28, 2023 22:27
/// called.
/// - forcesBreakBeforeRightParen: Whether a break should be required before the right paren
/// when the right paren is on a different line than the corresponding left paren.
private func arrangeEnumCaseParameterClause(
Copy link
Member Author

Choose a reason for hiding this comment

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

I just duplicated this method for EnumCaseParameterClauseSyntax. Alternatives would be

  1. Make this function take a leftParen: TokenSyntax, isEmpty: Bool, rightParen: TokenSyntax so the logic can be shared with arrangeParameterClause
  2. Create a protocol that offers the three parameters described in (1) and make both ParameterClauseSyntax and EnumCaseParameterClauseSyntax conform to that

(2) seemed like quite an overkill to me and (1) seemed ugly as well. Let me know if you’ve got preferences @allevato.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine; the function is small enough that duplicating it is probably better than overengineering a generic thing that would only be used in one extra place.

Copy link
Member Author

@ahoppen ahoppen Mar 28, 2023

Choose a reason for hiding this comment

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

That’s what I thought as well. But good to hear you agree with me.

/// called.
/// - forcesBreakBeforeRightParen: Whether a break should be required before the right paren
/// when the right paren is on a different line than the corresponding left paren.
private func arrangeEnumCaseParameterClause(
Copy link
Member

Choose a reason for hiding this comment

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

This is fine; the function is small enough that duplicating it is probably better than overengineering a generic thing that would only be used in one extra place.

@ahoppen ahoppen force-pushed the ahoppen/split-function-parameter branch from 2b62c75 to 0d22a27 Compare March 31, 2023 04:51
@ahoppen
Copy link
Member Author

ahoppen commented Mar 31, 2023

@allevato I have needed to duplicate some more code to make sure closure and enum case parameters are correctly formatted. Could you take another look whether you think it makes sense to unify some of these methods? I still think that it might be too much overhead but you decide.

@allevato
Copy link
Member

I still think the duplication is small enough that it's fine to have it for now, especially since all of these param lists are subtly different. If I want to revisit it in the future I can do that as part of a larger refactoring, but no reason to block you on it.

…r function parameters, closure parameters and enum parameters

Companion of swiftlang/swift-syntax#1455
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