Skip to content

[NFC] Rename TokenConsumptionHandle.missing to .tokenIsMissing #1405

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

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

WowbaggersLiquidLunch
Copy link
Contributor

@WowbaggersLiquidLunch WowbaggersLiquidLunch commented Mar 12, 2023

This clarifies what is missing at the use site. Also according the the API design guidelines:

Uses of Boolean methods and properties should read as assertions about the receiver when the use is nonmutating, e.g. x.isEmpty, line1.intersects(line2).

This clarifies what is missing at the use site. Also according the the API design guidelines:

> **Uses of Boolean methods and properties should read as assertions about the receiver** when the use is nonmutating, e.g. `x.isEmpty`, `line1.intersects(line2)`.
@WowbaggersLiquidLunch WowbaggersLiquidLunch changed the title [NFC] Rename TokenConsumtionHandle.missing to .isMissing to fit the API design guidelines [NFC] Rename TokenConsumtionHandle.missing to .tokenIsMissing Mar 12, 2023
@WowbaggersLiquidLunch
Copy link
Contributor Author

@swift-ci please test

@WowbaggersLiquidLunch WowbaggersLiquidLunch changed the title [NFC] Rename TokenConsumtionHandle.missing to .tokenIsMissing [NFC] Rename TokenConsumptionHandle.missing to .tokenIsMissing Mar 12, 2023
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.

Renaming missing -> isMissing makes sense to me. I’m not sure if we need to include token in there because at least to me it’s obvious that we’re talking about a token since only tokens can be missing in SwiftSyntax and this is a TokenConsumptionHandle. What did you think might have been missing? If it was ambiguous to you, I’m happy to change my mind.

@WowbaggersLiquidLunch
Copy link
Contributor Author

this is a _Token_ConsumptionHandle

When I was first reading the source, all I saw was that all the handles were named just handles. It wasn't obvious to me that they were instances of _Token_ConsumptionHandle. I only figured out that it's the token that's missing when I jumped to definition and read the documentation. I wasn't able to deduce it without reading the documentation, because I didn't know what the handles were for to begin with. So it would be confusing and doesn't seem obvious to me (and especially to new contributors I guess) that the receiver/subject of isMissing is the token not the handle.

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.

Ah, with that explanation the rename makes sense to me. Thanks.

@ahoppen ahoppen merged commit 6a2fa62 into swiftlang:main Mar 20, 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.

2 participants