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

Chunk improvements #3274

Merged
merged 52 commits into from
Aug 28, 2023
Merged

Chunk improvements #3274

merged 52 commits into from
Aug 28, 2023

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Aug 13, 2023

Fixes #3271


Improvements

  • Chunk.asJava.
  • Chunk.asSeq.
  • More Chunk operations like contains & exists (also overridden on the Foldable instance).
  • Add Chunk.javaList to create chunks based on a Java ju.List .
  • Add Chunk.from to accept any stdlib collection, and deprecate specific factories.
  • Improve Chunk.Queue :+, +:, and ++ methods.

As always, I look forward to your feedback. Especially related to naming, scaladoc, and tests.

Comment on lines 25 to 28
/** Views this Chunk as a Scala immutable Seq.
* Contrary to all methods that start with _"to"_ (e.g. {{toVector}}, {{toArray}}),
* this method does not copy data.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, not totally sure, that my Scaladoc formatting is wrong here.

.sbtopts Outdated Show resolved Hide resolved
@BalmungSan BalmungSan changed the title WIP: Chunk as Seq WIP: Chunk improvements Aug 14, 2023
Comment on lines 1189 to 1191
if (c.isEmpty) this
else
c match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (c.isEmpty) this
else
c match {
if (c.isEmpty) this
else if (this.isEmpty) c
else
c match {

and the same recommendation for :+ down below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that ++ can simply call to this method (i.e. +:) because they both seem doing the same thing, only the return type is different (++ returns Chunk whereas +: returns Queue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@satorg I made the changes in a slightly different way, in order to minimize the amount of allocations.

@filipwiech
Copy link

For what it's worth the changes look good to me based on my limited knowledge, so take it with a grain of salt. 😉 The only thing missing from my point of view are unit tests, would be great to cover the newly introduced functionality:

  • New Chunk methods: contains, count, exists.
  • Some basic asSeq usage.
  • Some basic asJava usage.
  • New Chunk.from factory.
  • New Chunk.javaList factory.

@BalmungSan
Copy link
Contributor Author

BalmungSan commented Aug 27, 2023

@mpilquist @armanbilge I think the PR is ready for review, sorry for taking too long to add the tests.
Let me know if you have more ideas for things to test.

cc @satorg @filipwiech -- Thanks for the review comments :)

@mpilquist mpilquist merged commit 1c4939f into typelevel:main Aug 28, 2023
14 checks passed
@BalmungSan BalmungSan deleted the chunk-as-seq branch August 28, 2023 16:57
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.

Provide a asSeq facade for Chunk
5 participants