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

replace java streams with generators #938

Merged

Conversation

@marek1840
Copy link
Collaborator

commented Sep 23, 2019

Previously, we created a wrapper for file walking and children listing to automatically close the underlying java stream. It lacked the flexibility and ease of use of standard collections.
Now, we use the geny library to close the stream automatically. It also provides a subset of scala collection methods (forall, map, etc.)

This is based on a previous work of @olafurpg

@marek1840 marek1840 changed the title replace custom file-walking with generators replace java streams with generators Sep 23, 2019
@marek1840 marek1840 force-pushed the marek1840:migrate-java-streams-to-generators branch 2 times, most recently from 6101ddf to b54291b Sep 23, 2019
Copy link
Collaborator

left a comment

LGTM

@marek1840 marek1840 force-pushed the marek1840:migrate-java-streams-to-generators branch 2 times, most recently from b321537 to 392c3f8 Sep 23, 2019
Copy link
Member

left a comment

This is a good idea! I'm not sure I like the AbsolutePath.{walk,list} extension methods. I feel like it would be better with a JFiles.walk(AbsolutePath) method on an object. Any thoughts on that?

}

def walk: Generator[AbsolutePath] = {
if (exists) Files.walk(path.toNIO).asScala.map(AbsolutePath(_))

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 23, 2019

Member

Looks like there's an implicit conversion going on here and the stream won't be closed. I would use the Geny.selfClosing constructor to ensure that it's properly closed.

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 23, 2019

Author Collaborator

streams don't have an implicit conversion with asScala - that is my own addition.
It tries to mimick the scala conversion to avoid any surprises on the callee site (like: "what was the name of that special method for streams?") - I want it to be as seamless as possible.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 23, 2019

Member

Aha, I'm sorry I didn't see that one. Looks great 👍

@marek1840 marek1840 force-pushed the marek1840:migrate-java-streams-to-generators branch from 392c3f8 to e765ebc Sep 23, 2019
@marek1840

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 23, 2019

I'm not sure I like the AbsolutePath.{walk,list} extension methods. I feel like it would be better with a JFiles.walk(AbsolutePath) method on an object.

I think it fits. there already is a method parent in AbsolutePath, so why just make it easier for walking the tree in just a one direction? I think there is no gain in having separate object for those special cases if they are so similar to what is already available in AbsolutePath

@marek1840 marek1840 marked this pull request as ready for review Sep 23, 2019
olafurpg and others added 3 commits Sep 2, 2019
Previously, we created a wrapper for file walking and children listing
to automatically close the underlying java stream, but it lacked the
flexibility and ease of use of standard collections.
Now, we use the geny library to close the stream automatically. It also provides a
subset of scala collection methods (forall, map, etc.)
the immediateChildren already handles that case
@marek1840 marek1840 force-pushed the marek1840:migrate-java-streams-to-generators branch from e765ebc to 35ce5fc Sep 23, 2019
Copy link
Member

left a comment

I'm fine with keeping AbsolutePath.{walk,list} extension methods, I agree it's consistent with other extension methods like .readAllBytes etc. LGTM 👍 Thank you for doing this nice cleanup 🙏

@marek1840 marek1840 merged commit e551ab8 into scalameta:master Sep 23, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20190923.11 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.