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

Ability to create Bytes shader function #6

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

joan38
Copy link
Contributor

@joan38 joan38 commented Aug 19, 2020

Related to the discussion in com-lihaoyi/mill#947 (comment)

@joan38 joan38 force-pushed the iterable branch 2 times, most recently from 15d96b4 to 6a422dd Compare August 19, 2020 20:59
@joan38 joan38 changed the title Mapping as Iterable instead of a Seq to allow for larger collection options Iterable instead of a Seq to allow for larger collection options and shader function Aug 19, 2020
@joan38 joan38 force-pushed the iterable branch 3 times, most recently from 8ae6e73 to 479b3f6 Compare August 20, 2020 08:16
@joan38 joan38 changed the title Iterable instead of a Seq to allow for larger collection options and shader function Ability to create InputStream and Bytes shader function Aug 20, 2020
Copy link

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

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

I opened #7, that includes the changes here, to address the comments I added.

(inputStream, mapping) =>
if (rules.isEmpty) Some(inputStream -> mapping)
else {
bytcodeShader(rules, verbose)(readAllBytes(inputStream), mapping).map { case (bytes, newMapping) =>

Choose a reason for hiding this comment

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

The bytcodeShader(rules, verbose) can be factored, to avoid re-converting the rules.

if (rules.isEmpty) Some(inputStream -> mapping)
else {
bytcodeShader(rules, verbose)(readAllBytes(inputStream), mapping).map { case (bytes, newMapping) =>
new ByteArrayInputStream(bytes) { override def close(): Unit = inputStream.close() } -> newMapping

Choose a reason for hiding this comment

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

Why close inputStream via this one? I'm not sure that makes sense, as inputStream is read before that (via readAllBytes(inputStream)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we need to maintain the close chain? Otherwise we risk of leaving open InputStream.

val shadedInputStream =
if (proc.process(entry)) Some(entry.data -> entry.name)
else None
shadedInputStream.filterNot(a => excludes.contains(a._2))

Choose a reason for hiding this comment

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

That condition can be pushed down, so that proc.process isn't called.

@joan38 joan38 mentioned this pull request Aug 20, 2020
@joan38 joan38 changed the title Ability to create InputStream and Bytes shader function Ability to create Bytes shader function Aug 20, 2020
@joan38
Copy link
Contributor Author

joan38 commented Aug 21, 2020

Alright, so we are good to go with this version @eed3si9n @alexarchambault ?

@eed3si9n
Copy link
Owner

I'll defer to Alex's review, and merge.

@eed3si9n eed3si9n merged commit cf68c46 into eed3si9n:master Aug 21, 2020
@eed3si9n
Copy link
Owner

@joan38 joan38 deleted the iterable branch August 21, 2020 18:32
@joan38
Copy link
Contributor Author

joan38 commented Aug 21, 2020

Thanks @eed3si9n

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.

3 participants