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.

Sign up for GitHub

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

Jump to bottom

chore: Add identity function. #1671

Merged
merged 1 commit into from
Jan 6, 2025
Merged

chore: Add identity function. #1671

merged 1 commit into from
Jan 6, 2025

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 4, 2025

Motivation:
Make life easier for Java developers without the i -> i

Modification:
Add an identity helper function.

Result:
Clean code.

@He-Pin He-Pin added the t:stream Pekko Streams label Jan 4, 2025
@He-Pin He-Pin added this to the 1.2.0 milestone Jan 4, 2025
mdedetrich
mdedetrich requested changes Jan 4, 2025
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

This is pointless? The Java identity function already exists in JDK 1.8 as java.util.function.Function.identity.

This might be useful for versions of JDK before 1.8 but Pekko doesn't support those JDK's

@He-Pin
Copy link
Member Author

He-Pin commented Jan 4, 2025

@mdedetrich Yes and no, see below:
image

@pjfanning
Copy link
Contributor

pjfanning commented Jan 4, 2025 •
edited
Loading

Can you translate the IDE issue into English? I prefer not to have images - if you paste the text (even the Chinese text), I can get a tool to translate it. If you paste images, I need an OCR tool and I don't have one available.

@He-Pin He-Pin requested a review from mdedetrich January 4, 2025 10:01
@He-Pin
Copy link
Member Author

He-Pin commented Jan 4, 2025

Sorry, the java.util.function.Function#identity returns a java.util.function.Function, even these methods share the same name, but returns two different types.

@pjfanning
Copy link
Contributor

pjfanning commented Jan 4, 2025 •
edited
Loading

I'm ok with adding this if it helps. I would like to still have some of tests use ConstantFun.javaIdentityFunction() for regression testing purposes though.

He-Pin
He-Pin commented Jan 4, 2025
View reviewed changes
actor-tests/src/test/java/org/apache/pekko/util/ConstantFunJavaTest.java
} catch (Exception e) {
Assert.fail("Exception thrown: " + e.getMessage());
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

added a dedicated test @pjfanning

@He-Pin
Copy link
Member Author

He-Pin commented Jan 5, 2025

@mdedetrich would you like talking another look, thanks

raboof
raboof approved these changes Jan 6, 2025
View reviewed changes
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.

unfortunate to have all those function types, but given those this seems like a nice utility

@He-Pin
Copy link
Member Author

He-Pin commented Jan 6, 2025

@mdedetrich Would you like to unblock this, thanks.

mdedetrich
mdedetrich approved these changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin He-Pin merged commit 8f6f97a into apache:main Jan 6, 2025
9 checks passed
@He-Pin He-Pin deleted the identity branch January 6, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdedetrich mdedetrich mdedetrich approved these changes

@raboof raboof raboof approved these changes

Assignees
No one assigned
Labels
t:stream Pekko Streams
Projects
None yet
Milestone
1.2.0-M1
Development

Successfully merging this pull request may close these issues.
4 participants
@He-Pin @pjfanning @raboof @mdedetrich