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

Add java.io.FilterReader #4912

Merged
merged 1 commit into from Nov 3, 2023
Merged

Add java.io.FilterReader #4912

merged 1 commit into from Nov 3, 2023

Conversation

ekrich
Copy link
Contributor

@ekrich ekrich commented Nov 1, 2023

No description provided.

javalib/src/main/scala/java/io/FilterReader.scala Outdated Show resolved Hide resolved
@@ -364,3 +364,10 @@ class InputStreamReaderTest {
assertThrows(classOf[IOException], r.mark(0))
}
}

class FilterReaderTest {
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit light, as far as tests goes. Could we at least test that giving it an actual reader as argument allows to call methods of that reader through the filter proxy?

Copy link
Member

Choose a reason for hiding this comment

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

Also, consider putting this in a dedicated file. I know the current file bundles a lot of them, but that's not great, and we should try not to perpetuate past mistakes. 😅

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 was sort of expecting the nice reused tests like in Collections :-)

I can put the others in separate files as well if you like?

Copy link
Member

Choose a reason for hiding this comment

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

I can put the others in separate files as well if you like?

If you feel like it, why not :)

@@ -364,3 +364,4 @@ class InputStreamReaderTest {
assertThrows(classOf[IOException], r.mark(0))
}
}

Copy link
Member

@sjrd sjrd Nov 2, 2023

Choose a reason for hiding this comment

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

Spurious change here, but otherwise LGTM.

Please squash all the commits in a single one with a good commit message. (except separating the existing tests in multiple files; that should be its own commit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be ok now.

@sjrd sjrd merged commit 2fa0a0a into scala-js:main Nov 3, 2023
3 checks passed
@ekrich ekrich deleted the topic/reader branch November 3, 2023 13:37
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.

None yet

3 participants