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

Use Java 8's Consumer interface. #1870

Merged
merged 3 commits into from
Jan 6, 2023
Merged

Conversation

Isira-Seneviratne
Copy link
Contributor

  • Use the Consumer interface introduced in Java 8, as it can be backported for older versions of Android via core library desugaring.

@jhy
Copy link
Owner

jhy commented Jan 5, 2023

Thanks, but can't merge with the build failures. Is there an update mojo sniffer?

And would need to make sure the API change is backwards compatible too.

@Isira-Seneviratne
Copy link
Contributor Author

Thanks, but can't merge with the build failures. Is there an update mojo sniffer?

I didn't change anything else, so I'm not sure why the build is failing.

And would need to make sure the API change is backwards compatible too.

With existing calls to forEach?

@jhy
Copy link
Owner

jhy commented Jan 5, 2023

The build fails because we use the Mojo Animal Sniffer plugin to verify that all APIs used are in the appropriate Java and Android language levels. It doesn't believe that the Consumer interface is available on Android -- which is why I added that internal workaround.

The error in the build is:

Error:  /home/runner/work/jsoup/jsoup/src/main/java/org/jsoup/nodes/Node.java:665: Undefined reference: void java.util.function.Consumer.accept(Object)
Error:  /home/runner/work/jsoup/jsoup/src/main/java/org/jsoup/nodes/Element.java:1825: Undefined reference: void java.util.function.Consumer.accept(Object)
Error:  Failed to execute goal org.codehaus.mojo:animal-sniffer-maven-plugin:1.22:check (animal-sniffer) on project jsoup: Signature errors found. Verify them and ignore them with the proper annotation if needed. -> [Help 1]

The check is defined in the POM:

jsoup/pom.xml

Lines 74 to 78 in 29be991

<signature>
<groupId>net.sf.androidscents.signature</groupId>
<artifactId>android-api-level-10</artifactId>
<version>2.3.3_r2</version>
</signature>

I don't see an update to that signature definition that supports the APIs brought in by new desugaring implementation. So we would need to find a different way to do those tests. Or allow-list the API explicitly, or something.

@Isira-Seneviratne Isira-Seneviratne force-pushed the Java_Consumer branch 2 times, most recently from 9ee71c4 to 6e77a00 Compare January 5, 2023 10:03
@Isira-Seneviratne
Copy link
Contributor Author

I added an ignore rule for the Mojo Animal Sniffer plugin.

@jhy
Copy link
Owner

jhy commented Jan 5, 2023

Cool - the next issue to resolve is the API backwards compat issue - the interface has changed:

Error:  Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.17.1:cmp (default) on project jsoup: There is at least one incompatibility: org.jsoup.helper.Consumer.accept(java.lang.Object):METHOD_REMOVED,org.jsoup.helper.Consumer:SUPERCLASS_REMOVED,org.jsoup.helper.Consumer:CLASS_REMOVED,org.jsoup.helper.Consumer:CLASS_GENERIC_TEMPLATE_CHANGED,org.jsoup.nodes.CDataNode:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Comment:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.DataNode:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Document:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.DocumentType:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Element.forEach(org.jsoup.helper.Consumer):METHOD_REMOVED,org.jsoup.nodes.Element.forEachNode(org.jsoup.helper.Consumer):METHOD_REMOVED,org.jsoup.nodes.Element:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.FormElement:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Node.forEachNode(org.jsoup.helper.Consumer):METHOD_REMOVED,org.jsoup.nodes.PseudoTextElement:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.TextNode:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.XmlDeclaration:METHOD_REMOVED_IN_SUPERCLASS -> [Help 1]

I think we can deal with that by making the jsoup Consumer extend from the Java Consumer, and mark it deprecated for removal in the next release.

You can run these tests locally with mvn verify and ensure they pass before pushing.

@Isira-Seneviratne
Copy link
Contributor Author

Cool - the next issue to resolve is the API backwards compat issue - the interface has changed:

Error:  Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.17.1:cmp (default) on project jsoup: There is at least one incompatibility: org.jsoup.helper.Consumer.accept(java.lang.Object):METHOD_REMOVED,org.jsoup.helper.Consumer:SUPERCLASS_REMOVED,org.jsoup.helper.Consumer:CLASS_REMOVED,org.jsoup.helper.Consumer:CLASS_GENERIC_TEMPLATE_CHANGED,org.jsoup.nodes.CDataNode:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Comment:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.DataNode:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Document:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.DocumentType:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Element.forEach(org.jsoup.helper.Consumer):METHOD_REMOVED,org.jsoup.nodes.Element.forEachNode(org.jsoup.helper.Consumer):METHOD_REMOVED,org.jsoup.nodes.Element:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.FormElement:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.Node.forEachNode(org.jsoup.helper.Consumer):METHOD_REMOVED,org.jsoup.nodes.PseudoTextElement:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.TextNode:METHOD_REMOVED_IN_SUPERCLASS,org.jsoup.nodes.XmlDeclaration:METHOD_REMOVED_IN_SUPERCLASS -> [Help 1]

I think we can deal with that by making the jsoup Consumer extend from the Java Consumer, and mark it deprecated for removal in the next release.

Yeah, I was thinking of adding overloaded methods for the separate consumer types, but your solution is much better.

@Isira-Seneviratne
Copy link
Contributor Author

I tried removing the accept method in the Jsoup Consumer, but Mojo gave an error saying the method had been removed, even though it's there in the parent interface.

In favour of java.util.function.Consumer
@deprecated Use {@link #forEachNode(Consumer)} instead.
*/
public Node forEachNode(org.jsoup.helper.Consumer<? super Node> action) {
Validate.notNull(action);

Check notice

Code scanning / CodeQL

Confusing overloading of methods

Method Node.forEachNode(..) could be confused with overloaded method [forEachNode](1), since dispatch depends on static types.
@jhy
Copy link
Owner

jhy commented Jan 6, 2023

OK, my commit has deprecated the old methods, and added a temporary deprecated overload for the old version. This ensures both source and binary compatibility in this version. The next version can remove these methods.

@jhy jhy merged commit 2c2cca9 into jhy:master Jan 6, 2023
@jhy jhy self-assigned this Jan 6, 2023
@jhy jhy added the improvement label Jan 6, 2023
@jhy jhy added this to the 1.15.4 milestone Jan 6, 2023
jhy added a commit that referenced this pull request Jan 6, 2023
@jhy
Copy link
Owner

jhy commented Jan 6, 2023

Thanks again! Have merged.

@Isira-Seneviratne Isira-Seneviratne deleted the Java_Consumer branch January 9, 2023 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants