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

Support primitives in Flow#collectType #1490

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

JD557
Copy link
Contributor

@JD557 JD557 commented Sep 20, 2024

Flow#collectType was not working as expected for primitives. This PR replaces the use of ClassTag#runtimeClass#instanceOf with ClassTag#unapply.

While the default implementation of ClassTag#unapply simply defers to ClassTag#runtimeClass#instanceOf, this is not the case for the ClassTags of primitives, which only override unapply.

See the IntManifest for an example:

https://github.com/scala/scala/blob/v2.13.14/src/library/scala/reflect/Manifest.scala#L223-L236

@pjfanning pjfanning added this to the 1.1.2 milestone Sep 20, 2024
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - merging, thanks @JD557

@pjfanning pjfanning merged commit 6ceb59b into apache:main Sep 20, 2024
9 checks passed
@pjfanning
Copy link
Contributor

Is this something that we should consider backporting to the 1.0.x relaeses?

@JD557
Copy link
Contributor Author

JD557 commented Sep 20, 2024

Is this something that we should consider backporting to the 1.0.x relaeses?

That's obviously up to you (the maintainers) to decide, but my gut feeling says that maybe not.

Mostly because it changes the behavior from Akka (I doubt anyone depended on this behavior, though) and it's not that hard to fix with a normal collect.

I actually imagine most people just use collect and don't even notice that this method exists (at least that's how I stumbled on the bug 😅)

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