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

Prevent Unrelated Casts on Child Choice Node #2184

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

Matejkob
Copy link
Contributor

@Matejkob Matejkob commented Sep 13, 2023

Resolves part of #2092

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Hmm, this does significantly reduce the readability in many cases. I didn’t consider that you couldn’t easily chain if case

After reading this, I think I changed my mind and we should offer non-deprecated casting methods to any option that the SyntaxChildChoices can have. And if one of that option is a base kind like DeclSyntax, we should offer a casting method to any DeclSyntaxProtocol.

@Matejkob
Copy link
Contributor Author

Hmm, this does significantly reduce the readability in many cases. I didn’t consider that you couldn’t easily chain if case…

I completely agree. That was my main concern with value binding in the first place. I haven't yet found the time to go through this pitch: https://forums.swift.org/t/pitch-is-case-expressions/64185, so I'm not sure why it hasn't gone through yet. The issue definitely should be addressed—it's a day-to-day problem for many folks.

After reading this, I think I changed my mind and we should offer non-deprecated casting methods to any option that the SyntaxChildChoices can have. And if one of that option is a base kind like DeclSyntax, we should offer a casting method to any DeclSyntaxProtocol.

Sounds good to me. I'll try cook something

@ahoppen
Copy link
Member

ahoppen commented Sep 13, 2023

so I'm not sure why it hasn't gone through yet. The issue definitely should be addressed—it's a day-to-day problem for many folks.

I haven’t read the pitch completely either but I would imagine it’s just a matter of someone writing the official proposal and creating an implementation for it.

@Matejkob
Copy link
Contributor Author

Base on last John's post https://forums.swift.org/t/pitch-is-case-expressions/64185/69 the whole pitch is abandoned 😢

@Matejkob Matejkob changed the title Add deprecated warning for child choices node casts Prevent Unrelated Casts on Child Choice Node Sep 14, 2023
@Matejkob Matejkob force-pushed the prevent-casts-on-choice-nodes branch 2 times, most recently from 54e0aaf to d60a3e1 Compare September 14, 2023 17:38
@Matejkob
Copy link
Contributor Author

@ahoppen how about now? Usage is cleaner, but there is plenty of code more on the implementation part :/

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks good to me in general

@Matejkob Matejkob force-pushed the prevent-casts-on-choice-nodes branch 2 times, most recently from 2d6331e to 3590c97 Compare September 15, 2023 10:32
@ahoppen
Copy link
Member

ahoppen commented Sep 16, 2023

@swift-ci Please test

@kimdv
Copy link
Contributor

kimdv commented Sep 16, 2023

@swift-ci please test windows

@ahoppen
Copy link
Member

ahoppen commented Sep 17, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit d3a73af into swiftlang:main Sep 19, 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 this pull request may close these issues.

3 participants