Skip to content

Conversation

@ekrich
Copy link
Contributor

@ekrich ekrich commented Nov 1, 2023

No description provided.

}
}

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 :)

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
@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.

3 participants