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

Adding support for SequencedCollection, SequencedSet and SequencedMap from JDK 21 #3584

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

msamoylych
Copy link

Adding support for java.util.SequencedCollection, java.util.SequencedSet and java.util.SequencedMap from JDK 21 for @Singular annotation (@Builder)

@rspilker
Copy link
Collaborator

Thanks for the contribution! I haven't reviewed the whole code yet.

I did copy the java 22 gradle fix directly to master, thanks for that solution, because it's not part of this fix. Please remove it from this pull request.

@Rawi01
Copy link
Collaborator

Rawi01 commented Jan 15, 2024

  • Please add yourself to the AUTHORS file
  • You have disabled the Eclipse test. My guess is that you did this to avoid the error messages about missing classes. There are two ways to handle this:
    • Add the test, mark the errors as expected and call it a day
    • Add the current runtime to the eclipse compiler classpath. IIRC this will cause some split module errors. The last time I managed to workaround it but I think in this case we can no longer do so and have to change something somewhere
  • I think the mapping between the sorted guava type and the sequenced collections is incorrect. There also is no test for this, do we ever call this?
  • +1 for updating the website

@msamoylych
Copy link
Author

msamoylych commented Jan 16, 2024

You have disabled the Eclipse test. My guess is that you did this to avoid the error messages about missing classes. There are two ways to handle this:
Add the test, mark the errors as expected and call it a day
Add the current runtime to the eclipse compiler classpath. IIRC this will cause some split module errors. The last time I managed to workaround it but I think in this case we can no longer do so and have to change something somewhere

Eclipse IDE 2023-12 test uses JDK 21 unless specified compiler.compliance.level but in ant.yml is specified JDK 17 to run this test. Therefore BuilderSingularSequencedCollections test performs (// version 21:) and fails due to error messages about missing classes in JDK 17.
I added the test with expected errors

I think the mapping between the sorted guava type and the sequenced collections is incorrect. There also is no test for this, do we ever call this?

There are no other suitable classes in Guava now. I think BuilderSingularRedirectToGuava test is sufficient

@reneleonhardt
Copy link

Awesome contribution! Is the new code covered and ready to be merged after you added @Generated?

@msamoylych
Copy link
Author

No, there are some problems with test-eclipse: eclipse-202312, eclipse-202403, eclipse-202406. They execute on JDK 17 and so there are compile errors: new interfaces 'cannot be resolved to a type'

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.

4 participants